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

Provide HTML tag dynamic proxy #25195

Closed
dhh opened this Issue May 30, 2016 · 13 comments

Comments

Projects
None yet
6 participants
@dhh
Member

dhh commented May 30, 2016

ActionView::Helpers::TagHelper provides #tag and #content_tag for generating HTML tags, but feel outmodded with both lots of positional parameters and assuming XHTML as the default. I think we can fix both problems and reduce the API to a single proxy object in the vein of Builder.

# To produce <span class="bookmark"></span> before:
content_tag :span, nil, class: 'bookmark'
# After (because we will keep an index of tags that need closing, like <span>, vs those that do not, like <br>)
tag.span class: 'bookmark'

# To produce <div id="post_1"> before:
content_tag :div, post.title, id: dom_id(post)  
# After:
tag.div post.title, id: dom_id(post)

# To produce <br> before:
tag :br, nil, true
# After (becomes like span, we know this tag doesn't need closing)
tag.br

# Would be nice to support nesting too, so to produce <div id="header"><span>hello</span></div>, you'd:
tag.div(id: 'header') { |tag| tag.span 'hello' }

The backwards compatible upgrade path here should be easy enough because TagHelper#tag currently requires at least one parameter. So if none is passed, we can switch into this new mode.

@dhh dhh added the actionview label May 30, 2016

@dhh dhh added this to the 5.1.0 milestone May 30, 2016

@dhh dhh changed the title from Provide HTML tag proxy to Provide HTML tag dynamic proxy May 30, 2016

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward May 30, 2016

Member

The self-closing tags should be complaint to which spec?
As in HTML5, extensive, etc.

Member

vipulnsward commented May 30, 2016

The self-closing tags should be complaint to which spec?
As in HTML5, extensive, etc.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh May 30, 2016

Member

HTML5.

Member

dhh commented May 30, 2016

HTML5.

@vipulnsward

This comment has been minimized.

Show comment
Hide comment

vipulnsward added a commit to vipulnsward/rails that referenced this issue May 30, 2016

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 May 30, 2016

Member

Do we need to go through a deprecation cycle for this? Even though I think ActionView::Helpers::* are nodoc'd, this still feels like a pretty big API change to me, which could affect third party libraries 😬

Member

maclover7 commented May 30, 2016

Do we need to go through a deprecation cycle for this? Even though I think ActionView::Helpers::* are nodoc'd, this still feels like a pretty big API change to me, which could affect third party libraries 😬

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward May 30, 2016

Member

@maclover7 this shouldn't need a deprecation in first cycle- see vipulnsward@347e820 for crude impl.

Member

vipulnsward commented May 30, 2016

@maclover7 this shouldn't need a deprecation in first cycle- see vipulnsward@347e820 for crude impl.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh May 31, 2016

Member

No hard deprecation needed. The proxy is instantiated through a call that wasn't possible before. We should soft deprecate via docs though.

On May 30, 2016, at 23:31, Jon Moss notifications@github.com wrote:

Do we need to go through a deprecation cycle for this? Even though I think ActionView::Helpers::* are nodoc'd, this still feels like a pretty big API change to me, which could affect third part libraries 😬


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Member

dhh commented May 31, 2016

No hard deprecation needed. The proxy is instantiated through a call that wasn't possible before. We should soft deprecate via docs though.

On May 30, 2016, at 23:31, Jon Moss notifications@github.com wrote:

Do we need to go through a deprecation cycle for this? Even though I think ActionView::Helpers::* are nodoc'd, this still feels like a pretty big API change to me, which could affect third part libraries 😬


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@marekkirejczyk

This comment has been minimized.

Show comment
Hide comment
@marekkirejczyk

marekkirejczyk Jun 5, 2016

Contributor

I am attaching a PR with prototype implementation HTML tag dynamic proxy. This is draft far from being ready, more a starting point for discussion.
Feedback will be appreciated, let me know if direction is good and should I continue.

Contributor

marekkirejczyk commented Jun 5, 2016

I am attaching a PR with prototype implementation HTML tag dynamic proxy. This is draft far from being ready, more a starting point for discussion.
Feedback will be appreciated, let me know if direction is good and should I continue.

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Jun 5, 2016

Member

@marekkirejczyk I started this here- vipulnsward@347e820 a week ago, din't get time to work over it. Will take a look at your PR as well. Please do pursue it.

Member

vipulnsward commented Jun 5, 2016

@marekkirejczyk I started this here- vipulnsward@347e820 a week ago, din't get time to work over it. Will take a look at your PR as well. Please do pursue it.

@marekkirejczyk

This comment has been minimized.

Show comment
Hide comment
@marekkirejczyk

marekkirejczyk Jun 20, 2016

Contributor

I updated pull request with hopefully close-to-final implementation. Feedback will be appreciated :)

Contributor

marekkirejczyk commented Jun 20, 2016

I updated pull request with hopefully close-to-final implementation. Feedback will be appreciated :)

@marekkirejczyk

This comment has been minimized.

Show comment
Hide comment
@marekkirejczyk

marekkirejczyk Jun 21, 2016

Contributor

@dhh thanks for feedback, remarks has been fixed, questions answered, PR updated.

Contributor

marekkirejczyk commented Jun 21, 2016

@dhh thanks for feedback, remarks has been fixed, questions answered, PR updated.

marekkirejczyk added a commit to marekkirejczyk/rails that referenced this issue Jun 26, 2016

marekkirejczyk added a commit to marekkirejczyk/rails that referenced this issue Jun 27, 2016

@marekkirejczyk

This comment has been minimized.

Show comment
Hide comment
@marekkirejczyk

marekkirejczyk Jun 27, 2016

Contributor

New pull request added related to this topic: #25543

Contributor

marekkirejczyk commented Jun 27, 2016

New pull request added related to this topic: #25543

kaspth added a commit that referenced this issue Jun 27, 2016

Merge pull request #25543 from marekkirejczyk/tag_builder_25195
New syntax for tag helpers i.e. tag.br instead of tag('br') #25195
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2016

Member

@kaspth what is missing to close this issue?

Member

rafaelfranca commented Jun 27, 2016

@kaspth what is missing to close this issue?

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jun 28, 2016

Member

@rafaelfranca nothing, I simply forgot to do it 😁

Fixed in #25543

Member

kaspth commented Jun 28, 2016

@rafaelfranca nothing, I simply forgot to do it 😁

Fixed in #25543

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