Allow `f.select` to be called with a single hash containing options and HTML options #46629

ghiculescu · 2022-12-01T21:10:32Z

Motivation / Background

I do this a lot:

<%= select :post, :author, authors, required: true %>

It doesn't work; the required attribute is ignored! Instead, you need to do this:

<%= select :post, :author, authors, {}, required: true %>

It's hard to remember the right API, and it looks to me like a code smell. It looks even smellier when you end up with this:

<%= select :post, :author, authors, { include_blank: "Choose an option" }, { required: true } %>

Where this would be nicer, but again, the required attribute is ignored:

<%= select :post, :author, authors, include_blank: "Choose an option", required: true %>

Detail

This PR implements a special handling for required, multiple, and size HTML attributes so that these now do the same thing:

<%= select :post, :author, authors, include_blank: "Choose an option", required: true %>
<%= select :post, :author, authors, { include_blank: "Choose an option" }, { required: true } %>

Additional information

As proof I'm not the only person who makes this mistake, one of the tests in the Rails test suite was wrong! The test added in #40522 puts the multiple attribute in the wrong place and has the wrong assertion as as result. This PR includes a fix for the test.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
CI is passing.

skipkayhil · 2022-12-02T03:56:05Z

<%= select :post, :author, authors, {}, required: true %>

I wrote something like this just today after going through the same steps of forgetting that it needed a second options hash to work, so I appreciate you looking to solve this!

In my case, I was adding a class to a select so I don't think this change would help that. I wonder if there's a way we could take more advantage of the separation of positional and keywords args to make options more optional 🤔

ghiculescu · 2022-12-02T17:35:22Z

Hmm 🤔 if you maintained an allowlist of attributes for options, then you could push any other given option to html_options, if no HTML options are provided.

I think the allowlist would be %i[include_blank prompt disabled include_hidden selected], can you think of any others?

skipkayhil · 2022-12-03T23:25:36Z

I think the allowlist would be %i[include_blank prompt disabled include_hidden selected], can you think of any others?

I had a similar thought, and was actually surprised that the docs don't list the available options: https://edgeapi.rubyonrails.org/classes/ActionView/Helpers/FormOptionsHelper.html#method-i-select

I was also curious if a similar pattern could be applied to other form helpers with html_options, however the list of options is not always so short: https://edgeapi.rubyonrails.org/classes/ActionView/Helpers/DateHelper.html#method-i-date_select

Maybe this is a case where the change makes sense for select even if it isn't consistent with other tags?

ghiculescu · 2022-12-04T23:16:04Z

I wonder why distinction between options and html_options even exists. None of the other form helpers have html_options, just select tags.

casperisfine · 2022-12-05T07:42:23Z

Isn't there a backward compatibility issue here? What if an app has size or required as a valid <option value>?

ghiculescu · 2022-12-05T15:23:39Z

Those attributes don't get passed down to individual option tags, so I don't think so (but it's possible I've misunderstood what you mean...).

casperisfine · 2022-12-05T15:28:10Z

Nah, I'm the one who was confused.

ghiculescu · 2022-12-12T16:31:49Z

@casperisfine did you have any opinions on the change and/or on the allowlist ideal discussed above?

casperisfine · 2022-12-12T17:26:47Z

Not really, but I don't do much frontend these days.

But since no-one else reviewed it, I'll do it tomorrow. At first sight I don't see any issues with it, I'm just always a bit put of by "brittle" argument processing. But in this case I see how it's much more ergonomic so...

byroot

Ok, please rebase and I'll merge.

…nd HTML options I do this a lot: ```erb <%= select :post, :author, authors, required: true %> ``` It doesn't work; the `required` attribute is ignored! Instead, you need to do this: ```erb <%= select :post, :author, authors, {}, required: true %> ``` It's hard to remember the right API, and it looks to me like a code smell. It looks even smellier when you end up with this: ```erb <%= select :post, :author, authors, { include_blank: "Choose an option" }, { required: true } %> ``` Where this would be nicer, but again, the `required` attribute is ignored: ```erb <%= select :post, :author, authors, include_blank: "Choose an option", required: true %> ``` This PR implements a special handling for `required`, `multiple`, and `size` HTML attributes so that these now do the same thing: ```erb <%= select :post, :author, authors, include_blank: "Choose an option", required: true %> <%= select :post, :author, authors, { include_blank: "Choose an option" }, { required: true } %> ``` ps. as proof I'm not the only person who makes this mistake, one of the tests in the Rails test suite was wrong! The test added in rails#40522 puts the `multiple` attribute in the wrong place and has the wrong assertion as as result. This PR includes a fix for the test.

ghiculescu · 2022-12-13T17:45:34Z

actionview/CHANGELOG.md

+
+    *Alex Ghiculescu*
+
+*   Datetime form helpers (`time_field`, `date_field`, `datetime_field`, `week_field`, `month_field`) now accept an instance of Time/Date/DateTime as `:value` option.


FYI @kryzhovnik I fixed a typo here (Datatime -> Datetime).

ghiculescu · 2022-12-13T18:02:29Z

Implemented this in a small app. So much better already. Thanks @byroot !

morgoth · 2022-12-13T19:46:40Z

@ghiculescu If one wants to set some class on the select element, does it still require to write <%= select :post, :author, authors, {}, class: "some-css-class" %>?

ghiculescu · 2022-12-13T19:53:31Z

Right now, yes. What did you think of this idea?

morgoth · 2022-12-13T19:56:30Z

I think that would be much better.
With this change merged, it will be even more confusing as one will not be sure which options are allowed in one hash and which ones still require additional {} before.

ghiculescu · 2022-12-14T19:02:02Z

Started working on this. Just a note on something I've observed so far which might explain why options and html_options are separate hashes.

Both accept a :disabled key.

In the options hash, disabled is passed to options_for_select and is used to determine which <option>s should have the disabled attribute. It expects an array of strings.
In the html_options hash, disabled expects a boolean. If true, then the entire <select> gets the disabled attribute.

This means that it would not be possible to have an API where you never need to think about the additional hash.

Here's some tests for the two options, both pass on main:

  def test_select_with_disabled_html_options_and_disabled_selected_option
    @post = Post.new
    @post.category = "foo"
    assert_dom_equal(
      "<select class=\"disabled\" disabled=\"disabled\" name=\"post[category]\" id=\"post_category\"><option value=\"\" label=\" \"></option>\n<option selected=\"selected\" value=\"foo\">foo</option>\n<option value=\"bar\">bar</option></select>",
      select("post", "category", %w(foo bar), { prompt: true, include_blank: true }, { class: "disabled", disabled: true })
    )
  end

  def test_select_with_disabled_option
    @post = Post.new
    @post.category = "foo"
    assert_dom_equal(
      "<select name=\"post[category]\" id=\"post_category\"><option value=\"\" label=\" \"></option>\n<option selected=\"selected\" value=\"foo\">foo</option>\n<option disabled=\"disabled\" value=\"bar\">bar</option></select>",
      select("post", "category", %w(foo bar), { prompt: true, include_blank: true, disabled: ["bar"] })
    )
  end

casperisfine · 2022-12-15T09:04:10Z

Both accept a :disabled key.

Urk, this makes me want to revert to be honest, as this is extremely confusing. What do you think @ghiculescu ?

ghiculescu · 2022-12-15T14:36:50Z

I still think this PR is fine but not sure if we should go further. My main evidence (apart from me liking it) is that some of the tests in Rails made the same mistake.

Of course that’s all fairly subjective. I won’t be upset if you revert, but we should fix the broken test if you do (I can do that).

rails-bot bot added the actionview label Dec 1, 2022

ghiculescu mentioned this pull request Dec 1, 2022

[v6.1.0.rc1] Fix regression for select tag helper with array #40522

Merged

byroot approved these changes Dec 13, 2022

View reviewed changes

ghiculescu force-pushed the select-merge-options branch from 408ede3 to 49c2e51 Compare December 13, 2022 17:44

ghiculescu requested a review from byroot December 13, 2022 17:44

byroot merged commit f4f954f into rails:main Dec 13, 2022

ghiculescu commented Dec 13, 2022

View reviewed changes

ghiculescu deleted the select-merge-options branch December 13, 2022 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `f.select` to be called with a single hash containing options and HTML options #46629

Allow `f.select` to be called with a single hash containing options and HTML options #46629

ghiculescu commented Dec 1, 2022 •

edited

skipkayhil commented Dec 2, 2022

ghiculescu commented Dec 2, 2022

skipkayhil commented Dec 3, 2022

ghiculescu commented Dec 4, 2022

casperisfine commented Dec 5, 2022

ghiculescu commented Dec 5, 2022

casperisfine commented Dec 5, 2022

ghiculescu commented Dec 12, 2022

casperisfine commented Dec 12, 2022

byroot left a comment

ghiculescu Dec 13, 2022

ghiculescu commented Dec 13, 2022

morgoth commented Dec 13, 2022

ghiculescu commented Dec 13, 2022

morgoth commented Dec 13, 2022

ghiculescu commented Dec 14, 2022 •

edited

casperisfine commented Dec 15, 2022

ghiculescu commented Dec 15, 2022


		Alex Ghiculescu

		* Datetime form helpers (`time_field`, `date_field`, `datetime_field`, `week_field`, `month_field`) now accept an instance of Time/Date/DateTime as `:value` option.

Allow f.select to be called with a single hash containing options and HTML options #46629

Allow f.select to be called with a single hash containing options and HTML options #46629

Conversation

ghiculescu commented Dec 1, 2022 • edited

Motivation / Background

Detail

Additional information

Checklist

skipkayhil commented Dec 2, 2022

ghiculescu commented Dec 2, 2022

skipkayhil commented Dec 3, 2022

ghiculescu commented Dec 4, 2022

casperisfine commented Dec 5, 2022

ghiculescu commented Dec 5, 2022

casperisfine commented Dec 5, 2022

ghiculescu commented Dec 12, 2022

casperisfine commented Dec 12, 2022

byroot left a comment

Choose a reason for hiding this comment

ghiculescu Dec 13, 2022

Choose a reason for hiding this comment

ghiculescu commented Dec 13, 2022

morgoth commented Dec 13, 2022

ghiculescu commented Dec 13, 2022

morgoth commented Dec 13, 2022

ghiculescu commented Dec 14, 2022 • edited

casperisfine commented Dec 15, 2022

ghiculescu commented Dec 15, 2022

Allow `f.select` to be called with a single hash containing options and HTML options #46629

Allow `f.select` to be called with a single hash containing options and HTML options #46629

ghiculescu commented Dec 1, 2022 •

edited

ghiculescu commented Dec 14, 2022 •

edited