We want to allow aria-* to pass like data-* in tag options #13548

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
@krainboltgreene

Exactly like data-* attributes on HTML elements it is now common to
pass aria-* style attributes to tags (bootstrap for example). The
spec is defined here
http://rawgithub.com/w3c/aria-in-html/master/index.html

In this PR I’ve cleaned up some of the previous tag option formatting
to be a bit faster, smaller to reason, and opens up to future additions
of similar hash-like options. If for instance we wanted to add the
rails-* hash-like we’d just change HASHLIKE_ATTRIBUTE_KEYS to
[‘data’, ‘aria’, ‘rails’]

Kurtis Rainbolt-Greene
We want to allow aria-* to pass like data-* in tag options
Exactly like `data-*` attributes on HTML elements it is now common to
pass `aria-*` style attributes to tags (bootstrap for example). The
spec is defined here
http://rawgithub.com/w3c/aria-in-html/master/index.html

In this PR I’ve cleaned up some of the previous tag option formatting
to be a bit faster, smaller to reason, and opens up to future additions
of similar hash-like options. If for instance we wanted to add the
`rails-*` hash-like we’d just change `HASHLIKE_ATTRIBUTE_KEYS` to
`[‘data’, ‘aria’, ‘rails’]`
@krainboltgreene

This comment has been minimized.

Show comment Hide comment
@krainboltgreene

krainboltgreene Dec 30, 2013

The failures seem unrelated to my change?

The failures seem unrelated to my change?

+ end
+ end
+
+ def ordered_options(options, escape)

This comment has been minimized.

Show comment Hide comment
@egilburg

egilburg Dec 30, 2013

Contributor

Consider providing a benchmark for your changes, to ensure the new implementation isn't significantly slower.

Also, can use .flatten!, .compact! and sort! (all on separate lines, not chained) for better performance.

@egilburg

egilburg Dec 30, 2013

Contributor

Consider providing a benchmark for your changes, to ensure the new implementation isn't significantly slower.

Also, can use .flatten!, .compact! and sort! (all on separate lines, not chained) for better performance.

This comment has been minimized.

Show comment Hide comment
@krainboltgreene

krainboltgreene Dec 30, 2013

That's a good point, we don't really want to copy the array each time.

Where are benchmarks located?

@krainboltgreene

krainboltgreene Dec 30, 2013

That's a good point, we don't really want to copy the array each time.

Where are benchmarks located?

This comment has been minimized.

Show comment Hide comment
@egilburg

egilburg Dec 30, 2013

Contributor

I don't think Rails mainains benchmarks similar to normal tests, but you can include benchmarks as a GIST or comment.

See http://www.ruby-doc.org/stdlib-1.9.3/libdoc/benchmark/rdoc/Benchmark.html

@egilburg

egilburg Dec 30, 2013

Contributor

I don't think Rails mainains benchmarks similar to normal tests, but you can include benchmarks as a GIST or comment.

See http://www.ruby-doc.org/stdlib-1.9.3/libdoc/benchmark/rdoc/Benchmark.html

This comment has been minimized.

Show comment Hide comment
@krainboltgreene

krainboltgreene Apr 28, 2014

Someone needs to tell me how and where I do benchmarking in Rails.

@krainboltgreene

krainboltgreene Apr 28, 2014

Someone needs to tell me how and where I do benchmarking in Rails.

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Apr 30, 2014

Member

Actually the benchmarking doesn't really take place in Rails for now. You should isolate snippets of code and benchmark the time it takes to run (with the original code and the improved one).

You can just create a Ruby file with simple classes and benchmark the execution of a specific area with the Benchmark module as @egilburg has pointed. In the Benchmark module's documentation, there is an example comparing for, times and upto for instance.

@robin850

robin850 Apr 30, 2014

Member

Actually the benchmarking doesn't really take place in Rails for now. You should isolate snippets of code and benchmark the time it takes to run (with the original code and the improved one).

You can just create a Ruby file with simple classes and benchmark the execution of a specific area with the Benchmark module as @egilburg has pointed. In the Benchmark module's documentation, there is an example comparing for, times and upto for instance.

This comment has been minimized.

Show comment Hide comment
@krainboltgreene

krainboltgreene Apr 30, 2014

Alright, so if Rails isn't benchmarking anything officially then I'm probably not going to do it either.

@krainboltgreene

krainboltgreene Apr 30, 2014

Alright, so if Rails isn't benchmarking anything officially then I'm probably not going to do it either.

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Apr 30, 2014

Member

Actually Rails plans to run benchmarks but this not yet the case. However this is not the problem here ; the team is still aware of performances and your patch will be shipped more easily if you prove that you're not slowing the execution of this area of the code.

@robin850

robin850 Apr 30, 2014

Member

Actually Rails plans to run benchmarks but this not yet the case. However this is not the problem here ; the team is still aware of performances and your patch will be shipped more easily if you prove that you're not slowing the execution of this area of the code.

actionmailer/CHANGELOG.md
@@ -1,3 +1,7 @@
+* Support for aria-* style attributes similar to data-* HTML element attributes.

This comment has been minimized.

Show comment Hide comment
@vipulnsward

vipulnsward Jan 7, 2014

Member

@krainboltgreene you need to move this to actionview.

@vipulnsward

vipulnsward Jan 7, 2014

Member

@krainboltgreene you need to move this to actionview.

This comment has been minimized.

Show comment Hide comment
@krainboltgreene

krainboltgreene Jan 8, 2014

Noted, doing that + benchmarks and some cleanup tonight.

@krainboltgreene

krainboltgreene Jan 8, 2014

Noted, doing that + benchmarks and some cleanup tonight.

@krainboltgreene

This comment has been minimized.

Show comment Hide comment
@krainboltgreene

krainboltgreene Apr 28, 2014

Any updates on getting this merged would be wonderful.

Any updates on getting this merged would be wonderful.

@robin850 robin850 added this to the 4.2.0 milestone Apr 30, 2014

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Sep 3, 2014

Owner

Closed by #16796 with a simpler implementation. Thank you for the contribution.

Owner

rafaelfranca commented Sep 3, 2014

Closed by #16796 with a simpler implementation. Thank you for the contribution.

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

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