Fixing id generation on radio buttons. fixes #194#195
Conversation
| do: "#{id}_#{field}" | ||
| def input_id(name, field) when is_atom(name), | ||
| do: "#{name}_#{field}" | ||
| def input_id(name, field, value) do |
There was a problem hiding this comment.
I don't think we need to make this function public. What if we call it radio_button_id and keep it private for now?
There was a problem hiding this comment.
This is basically so I can do label(name, field, for: input_id(name, field, value)). Otherwise I gotta re-implement that again. Also this method is useful for checkbox collection (when you have name: "search[keys][] for example) and need unique ids and for attributes just like for radio buttons.
There was a problem hiding this comment.
I see, agreed. If that's the case, we need to add documentation to it, as it is effectively a separate function than input_id/2.
| def input_id(name, field) when is_atom(name), | ||
| do: "#{name}_#{field}" | ||
| def input_id(name, field, value) do | ||
| value_id = value |> String.downcase |> String.replace(~r/\W/, "_") |
There was a problem hiding this comment.
We should probably set the Unicode flag in the regex: ~r/\W/u.
There was a problem hiding this comment.
Downcasing is just to make it look consistent. "search_key_foo" looks better than "search_key_FoO". Gotcha on the unicode thing (coming from ruby I didn't know it's a thing)
There was a problem hiding this comment.
Let's remove downcase then. It is not necessary. If people want it downcase, then they can pass it explicitly using the input_id/3 function. :D
There was a problem hiding this comment.
Fair enough. Will update PR for tomorrow.
|
I have added some comments, thank you @GBH! |
|
@josevalim PR is amended |
|
❤️ 💚 💙 💛 💜 |
2 participants
Made a
input_id/3just so it's easier to generateforattribute on labels. Maybe it makes sense to expandlabelto allow value passing. Not sure. One thing at the time.