New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and cleanup output handlers #1441

Merged
merged 10 commits into from Oct 10, 2013

Conversation

Projects
None yet
5 participants
@ujifgc
Member

ujifgc commented Oct 9, 2013

Warning: this disables concatenating of - in Slim and Haml templates #1441 (comment).

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Oct 9, 2013

Member

@ujifgc Thank you!

+1

BTW, do we have to rewrite padrino-admin templates of slim?
I think we should replace - with =. e.g. - form_tag ..

Member

namusyaka commented Oct 9, 2013

@ujifgc Thank you!

+1

BTW, do we have to rewrite padrino-admin templates of slim?
I think we should replace - with =. e.g. - form_tag ..

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Oct 9, 2013

Member

Yes, padrino-admin pages are broken at this PR.

Member

ujifgc commented Oct 9, 2013

Yes, padrino-admin pages are broken at this PR.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Oct 9, 2013

Member

I fixed the broken templates.
Please use it if you like.

Member

namusyaka commented Oct 9, 2013

I fixed the broken templates.
Please use it if you like.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Oct 9, 2013

Member

Nice job @ujifgc and @namusyaka! really glad to see this cleanup. template handling, especially with capture has been half-broken for a long time now. Good to see this getting solidified for 0.12

Member

nesquena commented Oct 9, 2013

Nice job @ujifgc and @namusyaka! really glad to see this cleanup. template handling, especially with capture has been half-broken for a long time now. Good to see this getting solidified for 0.12

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Oct 9, 2013

Member

Indeed awesome work guys. Keep going.

Member

DAddYE commented Oct 9, 2013

Indeed awesome work guys. Keep going.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Oct 10, 2013

Member

The last commit 80a0e24 disables concatenating of - in Slim and Haml templates.

If we decide we could implement a flag to enable legacy behavior of Padrino template handlers and concatenate on - (though I would not).

If some legacy app wants to keep wrong behavior, the monkey patch would be:

For HamlHandler:

class Padrino::Helpers::OutputHelpers::HamlHandler
  def concat_to_template(text="")
    template.haml_concat(text)
    nil
  end 
end

For SlimHandler:

class Padrino::Helpers::OutputHelpers::SlimHandler
  def concat_to_template(text="")
    self.output_buffer << text
    nil
  end
end

Please, check your apps with this PR and comment. I think, it's ready.

Member

ujifgc commented Oct 10, 2013

The last commit 80a0e24 disables concatenating of - in Slim and Haml templates.

If we decide we could implement a flag to enable legacy behavior of Padrino template handlers and concatenate on - (though I would not).

If some legacy app wants to keep wrong behavior, the monkey patch would be:

For HamlHandler:

class Padrino::Helpers::OutputHelpers::HamlHandler
  def concat_to_template(text="")
    template.haml_concat(text)
    nil
  end 
end

For SlimHandler:

class Padrino::Helpers::OutputHelpers::SlimHandler
  def concat_to_template(text="")
    self.output_buffer << text
    nil
  end
end

Please, check your apps with this PR and comment. I think, it's ready.

@ujifgc ujifgc referenced this pull request Oct 10, 2013

Closed

Improve slim test #1439

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Oct 10, 2013

Member

If I understand correctly it means almost every padrino app using forms will need to tweak every - to = or their forms will be totally broken. Seems like 0.12.0 is as good a time as any to make this fairly major breaking change. What do you all think @padrino/core-members ?

Member

nesquena commented Oct 10, 2013

If I understand correctly it means almost every padrino app using forms will need to tweak every - to = or their forms will be totally broken. Seems like 0.12.0 is as good a time as any to make this fairly major breaking change. What do you all think @padrino/core-members ?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Oct 10, 2013

Member

Yep. A tweak of template block helpers from - to = or the monkey patch I provided. The monkey patch actually doesn't break anything except the test for concatenating -.

Member

ujifgc commented Oct 10, 2013

Yep. A tweak of template block helpers from - to = or the monkey patch I provided. The monkey patch actually doesn't break anything except the test for concatenating -.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Oct 10, 2013

Member

Yeah, we should document the monkeypatch in the blog post for 0.12.0 release (#1019)

Member

nesquena commented Oct 10, 2013

Yeah, we should document the monkeypatch in the blog post for 0.12.0 release (#1019)

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Oct 10, 2013

Contributor

I think the breaking change makes a lot of sense towards readability and predictability of the code going further. Excellent job guys! :) Thanks so much!!

Contributor

dariocravero commented Oct 10, 2013

I think the breaking change makes a lot of sense towards readability and predictability of the code going further. Excellent job guys! :) Thanks so much!!

ujifgc added a commit that referenced this pull request Oct 10, 2013

@ujifgc ujifgc merged commit 8bb8124 into master Oct 10, 2013

1 check passed

default The Travis CI build passed
Details

@ujifgc ujifgc deleted the fix-output-handlers branch Oct 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment