Skip to content

Handle :hr in keyword lists #469

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.

Sign up for GitHub

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

Closed
bkilshaw wants to merge 1 commit into from
Closed

Handle :hr in keyword lists #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/phoenix_html/form.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,11 @@ defmodule Phoenix.HTML.Form do
{value, options}
end

[acc | option(option_key, option_value, extra ++ options, selected_values)]
if option_key == :hr do
[acc | hr_tag()]
else
[acc | option(option_key, option_value, extra ++ options, selected_values)]
end

:hr, acc ->
[acc | hr_tag()]
Expand Down
13 changes: 13 additions & 0 deletions test/phoenix_html/form_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,19 @@ defmodule Phoenix.HTML.FormTest do
~s(<hr/>) <>
~s(<option value="new">New</option>)

assert options_for_select(
[
[key: "First", value: "first"],
[key: :hr, value: nil],
Copy link
Member

@josevalim josevalim Oct 23, 2025

Choose a reason for hiding this comment

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

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

Copy link
Author

@bkilshaw bkilshaw Oct 23, 2025

Choose a reason for hiding this comment

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

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

CleanShot 2025-10-23 at 14 44 32@2x

Copy link
Member

@josevalim josevalim Oct 24, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

@bkilshaw bkilshaw Oct 24, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@josevalim josevalim Oct 24, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ryanwinchester ryanwinchester Oct 24, 2025

Choose a reason for hiding this comment

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

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

Copy link
Author

@bkilshaw bkilshaw Oct 24, 2025

Choose a reason for hiding this comment

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

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!

[key: "Last", value: "last"]
],
nil
)
|> safe_to_string() ==
~s(<option value="first">First</option>) <>
~s(<hr/>) <>
~s(<option value="last">Last</option>)

assert options_for_select(
[
{"Open", ~U[2025-01-01 06:30:00.000000Z]},
Expand Down