Skip to content
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

Allow array as html attribute #2161

Merged
merged 2 commits into from Dec 9, 2017
Merged

Allow array as html attribute #2161

merged 2 commits into from Dec 9, 2017

Conversation

aeris
Copy link
Contributor

@aeris aeris commented Nov 26, 2017

Hi,

Currently, attributes only allow single string value, which force developer to manually generate string value from array, for example for dynamic classes generation.

<% @classes = %i[foo bar] %>
<%= image_tag 'foo.png', class: @classes.join(' ') %>

This patch allows to use array of strings or symbols for attributes values, as in Ruby on Rails.

<% @classes = %i[foo bar] %>
<%= image_tag 'foo.png', class: @classes %>

@aeris aeris changed the title Allow array as html class Allow array as html attribute Nov 26, 2017
@namusyaka namusyaka self-requested a review December 9, 2017 08:05
Copy link
Contributor

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aeris Thanks for your contribution. Before starting to review this, please take a look at broken tests.

@aeris
Copy link
Contributor Author

aeris commented Dec 9, 2017

Seems failure are mostly not caused by this patch. Disk quota problem, a test on padrino-mailer…
I just fix the compatibility trouble with ruby 1.9 and array notation.

@ujifgc
Copy link
Member

ujifgc commented Dec 9, 2017

Dope!

@namusyaka
Copy link
Contributor

escape_value seems that this is actually used to escape attribute values.
Although there is an example like the srcset attribute, basically this function will be conveniently usable.
Thanks for the patch.

Copy link
Contributor

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@namusyaka namusyaka merged commit c08410d into padrino:master Dec 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants