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

Already on GitHub? Sign in to your account

sanitize_dom_id #6015

Closed
davidslvto opened this Issue Apr 27, 2012 · 15 comments

Comments

Projects
None yet
5 participants

Hi,

The method sanitize_dom_id shouldn't have a regular expression instead of return the id?

Something like: [A-Za-z][-A-Za-z0-9_:.]*

In here:

candidate_id # TODO implement conversion to valid DOM id values

Reference: http://stackoverflow.com/questions/70579/what-are-valid-values-for-the-id-attribute-in-html

Owner

rafaelfranca commented Apr 27, 2012

Pull request please

dlt commented Apr 27, 2012

I can provide a PR if some one points me in the right direction.

Contributor

larzconwell commented Apr 27, 2012

No returning the id is fine because the layout template references the HTML5 doctype so we don't have to start with an [a-zA-Z] character.

Owner

rafaelfranca commented Apr 27, 2012

So we should remove the #TODO comment.

Contributor

larzconwell commented Apr 27, 2012

Yes we should, although I wonder if it does affect older browsers that don't comply to HTML5 spec. I doubt it would cause any problems but maybe. I don't have Windows or I would test it in IE6-8 ):

dlt commented Apr 27, 2012

Or maybe we should remove/rename the sanitize_dom_id method, since it doesn't do anything.

Contributor

larzconwell commented Apr 27, 2012

Yeah it literally does nothing, I think we should just remove it. Then record_key_for_dom_id needs to be changed to accommodate for the method removal.

def record_key_for_dom_id(record)
  record = record.to_model if record.respond_to?(:to_model)
  key = record.to_key
  key ? key.join('_') : key
end

Or maybe just return the key itself.

dlt commented Apr 27, 2012

Your version of the record_key_dom_id method seems quite good. If doesn't break any tests I think its a good idea for you to push it in a PR.

Contributor

larzconwell commented Apr 27, 2012

Hold on that method does some weird things at the end

  key ? key.join('_') : key

Why is it returning key if key isn't there??

  if key
    key.join('_')
  else
    key
  end

We might as well do

  key.join('_') if key

right?

dlt commented Apr 27, 2012

Well, maybe the calling method is expecting to use the returned value (key) as a string, so maybe it doesn't matter if you use a ternary operator or a if condition, since both will return nil and nil.to_s is an empty string.

Contributor

larzconwell commented Apr 27, 2012

Tests pass, I'm gonna make a PR with the key.join('_') if key version and remove the sanitize_dom_id method

dlt commented Apr 27, 2012

We might as well do something like:

  (key || "").join('_')

but let's not get into bikeshedding. ;-)

Contributor

larzconwell commented Apr 27, 2012

That actually looks pretty good! I'm just gonna keep the original, it seemed to work before so why not.

dlt commented Apr 27, 2012

+1

Closing in favor of the pull request. @larzconwell thanks for your work on that.

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