Handle :hr in keyword lists

[bkilshaw]

This fixes an issue where the :hr special case isn't handled by options_for_select when passing in a keyword list.

[josevalim]

Thanks but I don't think it is expected to be given as key?

[bkilshaw]

It's already handled in the other cases and referenced in the docs, but it's not currently handled when passing in a keyword list.

https://hexdocs.pm/phoenix_html/Phoenix.HTML.Form.html#options_for_select/3

[josevalim]

I see... I am really not sure if we should go ahead with this. I am not a big fan of the hr: nil, because ther eis ambiguity, it could have been an actual value, but I can understand why it is there as otherwise would have to convert the whole thing to a list of tuples. But in your example, the :hr usage is would just work, there is no reason to introduce ambiguity.

[bkilshaw]

Currently :hr works when passing in simple values and hr: nil works when using two element tuples, but there's no way to insert an hr tag when using keyword lists.

`options` is expected to be an enumerable which will be used to
generate each `option` element. The function supports different data
for the individual elements:

  * keyword lists - each keyword list is expected to have the keys
    `:key` and `:value`. Additional keys such as `:disabled` may
    be given to customize the option.
  * two-item tuples - where the first element is an atom, string or
    integer to be used as the option label and the second element is
    an atom, string or integer to be used as the option value
  * simple atom, string or integer - which will be used as both label and value
    for the generated select

Today I can do ["First", :hr, "Last"] and [first: "First", hr: nil, last: "Last"] but if I want to do something like this I'm out of luck:

options
|> Enum.map(fn option ->
   [key: option.key, value: option.value, disabled: disable_option?(option)]
end)
|> List.insert_at(0, [key: :hr, value: nil, disabled: true])

It's not really any more ambiguous than the currently supported hr: nil option.

I am not a big fan of the hr: nil, because ther eis ambiguity, it could have been an actual value

Agreed; I think supporting something like {:safe, "<hr />"} would probably make more sense in all scenarios since it's extremely unlikely that would ever be an intended key/value and it gives the user more control over what they want to stick in there.

[josevalim]

We should support something like this instead:

options
|> Enum.map(fn option ->
   [key: option.key, value: option.value, disabled: disable_option?(option)]
end)
|> List.insert_at(0, :hr)

But I agree a general mechanism to introduce any html snippet would be even better.

[ryanwinchester]

+1 allowing html snippets, plus deprecating :hr...

[bkilshaw]

We should support something like this instead:

options
|> Enum.map(fn option ->
   [key: option.key, value: option.value, disabled: disable_option?(option)]
end)
|> List.insert_at(0, :hr)

But I agree a general mechanism to introduce any html snippet would be even better.

Actually; this does already work. I must have tried List.insert_at(0, hr: nil) by accident.

Thanks!