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

HTML tag dynamic proxy - modern syntax for tag helpers, related to issue #25195 #25289

Closed
wants to merge 33 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@marekkirejczyk
Contributor

marekkirejczyk commented Jun 5, 2016

Prototype for new tag helper syntax

This is prototype implementation for @dhh's proposal in issue #25195 for HTML tag dynamic proxy.
The purpose of this PR is to discuss and gather feedback rather then being merged.

Example usage:

# 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' }
@rails-bot

This comment has been minimized.

rails-bot commented Jun 5, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @matthewd (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@simi

This comment has been minimized.

Contributor

simi commented Jun 5, 2016

Is there any reason why not to use already built builder for this implementaion?

@maclover7 maclover7 added the actionview label Jun 5, 2016

@marekkirejczyk

This comment has been minimized.

Contributor

marekkirejczyk commented Jun 6, 2016

@simi good question. One doubt I have is that builder is external dependency (https://github.com/jimweirich/builder) and second I am not sure which is better starting point - current tag helper code base or xmlbuilder code base. Current tag helper already have html related logic implemented (boolean attributes, tag prefixes, etc). On the other hand Builder::XMLMarkup has XML generation implemented. Not sure yet how to combine both.

VOID_ELEMENTS.merge(VOID_ELEMENTS.map(&:to_sym))
private

This comment has been minimized.

@dhh

dhh Jun 6, 2016

Member

Snip newline

def method_missing(called, *args, &block)
render_tag(called, args[0], args[1], &block)
end

This comment has been minimized.

@dhh

dhh Jun 6, 2016

Member

Snip newline

@dhh

This comment has been minimized.

Member

dhh commented Jun 6, 2016

If we're going to base the tag proxy off the existing helpers, then I don't think it makes sense to add Builder to the mix. As the code shows in this patch, that's really simple.

The only argument could be if we wanted to go the other way around. Reimplement everything to sit on top of a builder-backed tag proxy and have content_tag and friends call that. But I don't see a compelling reason to do that, primarily because I don't see a compelling reason to hard deprecate the existing usage of tag and content_tag. There's so much code written with that already that wouldn't be materially better off by being rewritten with the proxy tag.

The proxy tag is really just to make new code a little better. So I think the approach taken in this patch is sound.

@simi

This comment has been minimized.

Contributor

simi commented Jun 6, 2016

@marekkirejczyk builder is already actionview dependency, so I'm not sure if there's any reason to duplicate that.

@dhh

This comment has been minimized.

Member

dhh commented Jun 6, 2016

@simi Feel free to work on an alternative patch that uses Builder and see if it turns out better on the A/B than this patch. I'd consider that a refactoring that can happen after this has been merged in any case. They should be functionally equivalent.

@marekkirejczyk

This comment has been minimized.

Contributor

marekkirejczyk commented Jun 7, 2016

@simi @dhh Thank you for feedback, I'll continue current approach and let's see final result.

@marekkirejczyk

This comment has been minimized.

Contributor

marekkirejczyk commented Jun 19, 2016

I added implementation along with tests and documentation. I'll appreciate feedback :)

Notes and considerations:

  • Implementation is not based on old helpers now. That opens possibility for easy deprecation.
  • It supports all goodies from old helpers (options, disable escaping, using blocks in erb templates)
  • should produce code HTML5 compliant, close tags, but treat void elements accordingly
  • It is my first commit of that size, please be forgiving :)

@rafaelfranca rafaelfranca self-assigned this Jun 19, 2016

@marekkirejczyk marekkirejczyk changed the title from Protype of new tag helper syntax, i.e. tag.span class: 'bookmark' to HTML tag dynamic proxy - modern syntax for tag helpers, related to issue #25195 Jun 20, 2016

@marekkirejczyk

This comment has been minimized.

Contributor

marekkirejczyk commented Jun 20, 2016

Small fixes, typos and improvements added.

end
private

This comment has been minimized.

@dhh

dhh Jun 21, 2016

Member

Snip newline.

VOID_ELEMENTS = %w(base br col embed hr img input keygen link
meta param source track wbr).to_set

This comment has been minimized.

@dhh

dhh Jun 21, 2016

Member

Single space between elements.

def tag(name = nil, options = nil, open = false, escape = true)
return @tag_builder ||= TagBuilder.new(self) if name == nil
"<#{name}#{tag_options(options, escape) if options}#{open ? ">" : " />"}".html_safe

This comment has been minimized.

@dhh

dhh Jun 21, 2016

Member

Prefer an explicit if/else flow rather than early return. So:

if name.nil?
  @tag_builder ||= TagBuilder.new(self)
else
  "<#{name}#{tag_options(options, escape) if options}#{open ? ">" : " />"}".html_safe
end
def method_missing(called, *args, &block)
return tag_string(called, *args, &block) if ELEMENTS.include?(called)
super

This comment has been minimized.

@dhh

dhh Jun 21, 2016

Member

Use explicit if/else flow here as well.

has_content = block_given? || content_or_options.is_a?(String)
void_with_content = has_content && VOID_ELEMENTS.include?(name)
raise ArgumentError, "Void tag with content" if void_with_content
end

This comment has been minimized.

@dhh

dhh Jun 21, 2016

Member

IMO, we don't need this protection. If someone wants to bastardize a BR tag with content, that's their prerogative. We just do the right thing by default.

else
options_or_escape = true if options_or_escape.nil?
content_tag_string(name, "", content_or_options, options_or_escape)
end

This comment has been minimized.

@dhh

dhh Jun 21, 2016

Member

Turn this into a case statement for readability.

# void[https://www.w3.org/TR/html5/syntax.html#void-elements] element.
#
# ==== Options
# Like with traditional syntax the options hash can be used with

This comment has been minimized.

@kaspth

kaspth Jun 26, 2016

Member

Why are we referencing the traditional syntax here? Wouldn't it be best to assume people only know the new way?

# Like with traditional syntax the options hash can be used with
# attributes with no value (like disabled and readonly), which you can
# give a value of true in the options hash. You can use symbols or
# strings for the attribute names.

This comment has been minimized.

@kaspth

kaspth Jun 26, 2016

Member

Only symbols 😁 — but should we mention that? As long as the examples only use symbols, we could be in the clear.

# === Modern syntax
# Modern syntax follows one of two formats:
# tag.<name>(options)
# tag.<name>(content, options)

This comment has been minimized.

@kaspth

kaspth Jun 26, 2016

Member

Isn't it just one format with optional arguments?

# in options to disable attribute value escaping. The tag will be
# generated with related closing tag unless tag represents a
# void[https://www.w3.org/TR/html5/syntax.html#void-elements] element.
#

This comment has been minimized.

@kaspth

kaspth Jun 26, 2016

Member

I wanted to see if interspersing the examples inbetween documentation would read nicer.

# === Building HTML tags
# Builds HTML5 compliant tags with a tag proxy. Every tag can be built with:
#
#   tag.<tag name>(optional content, options)
#
# where tag name can be e.g. br, div, section, article, or any tag really.
#
# ==== Passing content
# Tags can pass content to embed within it:
#
#   tag.h1 'All shit fit to print' # => <h1>All shit fit to print</h1>
#
#   tag.div tag.p('Hello world!')  # => <div><p>Hello world!</p></div>
#
# Content can also be captured with a block. Great for ERB templates:
#
#   <%= tag.p do %>
#     The next great American novel starts here.
#   <% end %>
#   # => <p>The next great American novel starts here.</p>
#
# ==== Options
# Any passed options becomes attributes on the generated tag.
#
#   tag.section class: %w( kitties puppies )
#   # => <section class="kitties puppies"></section>
#
#   tag.section id: dom_id(@post)
#   # => <section id="<generated dom id>"></section>
#
# Pass true for any attributes that can render with no values like +disabled+.
#
#   tag.input type: 'text', disabled: true
#   # => <input type="text" disabled="disabled">
#
# HTML5 <tt>data-*</tt> attributes can be set with a single +data+ key
# pointing to a hash of sub-attributes.
#
# To play nicely with JavaScript conventions sub-attributes are dasherized.
#
#   tag.article data: { user_id: 123 }
#   # => <article data-user-id="123"></article>
#
# Thus <tt>data-user-id</tt> can be accessed as <tt>dataset.userId</tt>.
#
# Data attribute values are encoded to JSON, with the exception of strings, symbols and
# BigDecimals.
# This may come in handy when using jQuery's HTML5-aware <tt>.data()</tt>
# from 1.4.3.
#
#   tag.div data: { city_state: %w( Chigaco IL ) }
#   # => <div data-city-state="[&quot;Chicago&quot;,&quot;IL&quot;]"></div>
#
# The generated attributes are escaped by default, but it can be turned off with
# +escape_attributes+.
#
#   tag.img src: 'open & shut.png'
#   # => <img src="open &amp; shut.png">
#
#   tag.img src: 'open & shut.png', escape_attributes: false
#   # => <img src="open & shut.png">
#
# The tag builder respects
# [HTML5 void elements](https://www.w3.org/TR/html5/syntax.html#void-elements)
# if no content is passed, and omits closing tags for those elements.
#
#   # A standard element:
#   tag.div # => <div></div>
#
#   # A void element:
#   tag.br  # => <br>

What do you think, @marekkirejczyk, @rafaelfranca? 😁

This comment has been minimized.

@kaspth

kaspth Jun 26, 2016

Member

But I think we can look into that in a later PR which I'd be happy to open.

This comment has been minimized.

@marekkirejczyk

marekkirejczyk Jun 26, 2016

Contributor

Looks awsome!

This comment has been minimized.

@kaspth

kaspth Jun 26, 2016

Member

Fantastic! Feel free to incorporate it into this PR if you want ❤️

@kaspth

This comment has been minimized.

Member

kaspth commented Jun 26, 2016

@marekkirejczyk can you squash your commits down to 1? 😁

@kaspth

This comment has been minimized.

Member

kaspth commented Jun 26, 2016

Just realized we likely want to dasherize tag names to support multi-word web components natively:

tag.img_slider # => <img-slider></img-slider>
@marekkirejczyk

This comment has been minimized.

Contributor

marekkirejczyk commented Jun 26, 2016

@kaspth I'll find some time tomorrow to squash commits, do the dasherize and include documentation. And ... if you find anything else hopefully that too :) Thanks for feedback again :D

@marekkirejczyk

This comment has been minimized.

Contributor

marekkirejczyk commented Jun 27, 2016

@kaspth I created new pull request with commits squashed. Documentation is updated. Tag names dasherized. Please check documentation one more time. Also added you to changelog :) Let continue there if needed.

@kaspth kaspth added this to the 5.1.0 milestone Jun 27, 2016

@kaspth

This comment has been minimized.

Member

kaspth commented Jun 27, 2016

@marekkirejczyk you don't have to open a new pr, you can just force push to this branch :)

Though I went ahead and merged since it's ready #25543. Thanks so much for this! It was a blast helping here and I hope we get to see you contributing again.

In fact, you could check if @vipulnsward is interested in getting help with #25197 😁

@kaspth kaspth closed this Jun 27, 2016

@marekkirejczyk

This comment has been minimized.

Contributor

marekkirejczyk commented Jun 28, 2016

@kaspth It was a blast to be helped :) I actually already have a working prototype of #25197, will push it today or tomorrow evening.

@kaspth

This comment has been minimized.

Member

kaspth commented Jun 28, 2016

Sweet!

@vipulnsward

This comment has been minimized.

Member

vipulnsward commented Jun 28, 2016

@jgarber623

This comment has been minimized.

jgarber623 commented Feb 4, 2017

Is anyone else having trouble with this new syntax?

With a brand new Rails 5.0.1 app, adding the following to app/views/layouts/application.html.erb per the example in action_view/lib/helpers/tag_helper.rb:

<%= tag.br %>

I get an ArgumentError:

wrong number of arguments (given 0, expected 1..4)

All other examples from tag_helper.rb yield similar errors. Am I missing something obvious (a configuration setting, perhaps)?

@dhh

This comment has been minimized.

Member

dhh commented Feb 4, 2017

This is a Rails 5.1 feature. That's why.

@jgarber623

This comment has been minimized.

jgarber623 commented Feb 5, 2017

Ack, I'm sorry. I should've realized that! Thanks for the quick reply.

Should helpers (e.g. stylesheet_link_tag) be updated to use the new syntax as part of the 5.1 release?

@marekkirejczyk

This comment has been minimized.

Contributor

marekkirejczyk commented Feb 5, 2017

Sure, you be able to write:

tag.link rel: "stylesheet", type: "text/css", href: "theme.css"

However stylesheet_link_tag helper will remain unchanged.

@jgarber623

This comment has been minimized.

jgarber623 commented Feb 5, 2017

@marekkirejczyk Thanks for the quick reply!

However stylesheet_link_tag helper will remain unchanged.

To satisfy my own curiosity, why won't stylesheet_link_tag be updated to use the new syntax?

@kaspth

This comment has been minimized.

Member

kaspth commented Feb 5, 2017

@marekkirejczyk I think @jgarber623 means the underlying implementation of stylesheet_link_tag. :)

@jgarber623 I'd be willing to entertain a PR on that if we also update other helpers 👍 — though be wary as the PR teeters close to cosmetic commit grounds, which we don't accept. Here's more: #13771 (comment)

@jgarber623

This comment has been minimized.

jgarber623 commented Feb 5, 2017

I'd be willing to entertain a PR on that if we also update other helpers 👍 — though be wary as the PR teeters close to cosmetic commit grounds, which we don't accept.

@kaspth Fair enough and good to know! Given the team's stance on cosmetic commits, it seems more appropriate to change stylesheet_link_tag's implementation when the deprecated tag() code is removed from the codebase.

Thanks all for the info and hard work maintaining and improving Rails!

@kaspth

This comment has been minimized.

Member

kaspth commented Feb 6, 2017

Yup, that's even better 👍 — removing deprecated code is something our release managers do, so (unfortunately) please don't send a PR :)

Thanks all for the info and hard work maintaining and improving Rails!

Thanks for jumping in and helping out! ❤️

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