TEST_CODEOWNERS/rails
3
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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 https://github.com/rails/rails/pull/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.
This commit is contained in:
Alex Ghiculescu 2022-12-01 15:05:22 -06:00
parent ee5aec2040
commit 49c2e51808
3 changed files with 55 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
actionview/CHANGELOG.md
View File

@ -1,4 +1,22 @@
* Datatime form helpers (`time_field`, `date_field`, `datetime_field`, `week_field`, `month_field`) now accept an instance of Time/Date/DateTime as `:value` option.
* `select` can now be called with a single hash containing options and some HTML options
Previously this would not work as expected:
```erb
<%= select :post, :author, authors, required: true %>
```
Instead you needed to do this:
```erb
<%= select :post, :author, authors, {}, required: true %>
```
Now, either form is accepted, for the following HTML attributes: `required`, `multiple`, `size`.
*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.
Before:
```erb

4
actionview/lib/action_view/helpers/tags/base.rb
View File

@ -122,6 +122,10 @@ def sanitized_value(value)
def select_content_tag(option_tags, options, html_options)
html_options = html_options.stringify_keys
[:required, :multiple, :size].each do |prop|
html_options[prop.to_s] = options.delete(prop) if options.key?(prop) && !html_options.key?(prop.to_s)
end
add_default_name_and_id(html_options)
if placeholder_required?(html_options)

60
actionview/test/template/form_options_helper_test.rb
View File

@ -873,52 +873,56 @@ def test_select_with_nil_and_selected_option_as_nil
def test_select_with_array
@continent = Continent.new
@continent.countries = ["Africa", "Europe"]
assert_dom_equal(
%(<select name="continent[countries]" id="continent_countries"><option selected="selected" value="Africa">Africa</option>\n<option selected="selected" value="Europe">Europe</option>\n<option value="America">America</option></select>),
select("continent", "countries", %W(Africa Europe America), { multiple: true })
)
expected_with_hidden_field_and_multiple = %(<input name="continent[countries][]" type="hidden" value="" autocomplete="off"/><select name="continent[countries][]" id="continent_countries" multiple="multiple"><option selected="selected" value="Africa">Africa</option>\n<option selected="selected" value="Europe">Europe</option>\n<option value="America">America</option></select>)
expected_without_hidden_field = %(<select name="continent[countries]" id="continent_countries"><option selected="selected" value="Africa">Africa</option>\n<option selected="selected" value="Europe">Europe</option>\n<option value="America">America</option></select>)
assert_dom_equal(expected_with_hidden_field_and_multiple, select("continent", "countries", %W(Africa Europe America), { multiple: true }))
assert_dom_equal(expected_with_hidden_field_and_multiple, select("continent", "countries", %W(Africa Europe America), multiple: true))
assert_dom_equal(expected_with_hidden_field_and_multiple, select("continent", "countries", %W(Africa Europe America), {}, { multiple: true }))
assert_dom_equal(expected_without_hidden_field, select("continent", "countries", %W(Africa Europe America)))
end
def test_required_select
assert_dom_equal(
%(<select id="post_category" name="post[category]" required="required"><option value="" label=" "></option>\n<option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>),
select("post", "category", %w(abe mus hest), {}, { required: true })
)
expected = %(<select id="post_category" name="post[category]" required="required"><option value="" label=" "></option>\n<option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>)
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), {}, { required: true }))
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), required: true))
end
def test_required_select_with_include_blank_prompt
assert_dom_equal(
%(<select id="post_category" name="post[category]" required="required"><option value="">Select one</option>\n<option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>),
select("post", "category", %w(abe mus hest), { include_blank: "Select one" }, { required: true })
)
expected = %(<select id="post_category" name="post[category]" required="required"><option value="">Select one</option>\n<option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>)
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), { include_blank: "Select one" }, { required: true }))
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), include_blank: "Select one", required: true))
end
def test_required_select_with_prompt
assert_dom_equal(
%(<select id="post_category" name="post[category]" required="required"><option value="">Select one</option>\n<option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>),
select("post", "category", %w(abe mus hest), { prompt: "Select one" }, { required: true })
)
expected = %(<select id="post_category" name="post[category]" required="required"><option value="">Select one</option>\n<option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>)
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), { prompt: "Select one" }, { required: true }))
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), prompt: "Select one", required: true))
end
def test_required_select_display_size_equals_to_one
assert_dom_equal(
%(<select id="post_category" name="post[category]" required="required" size="1"><option value="" label=" "></option>\n<option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>),
select("post", "category", %w(abe mus hest), {}, { required: true, size: 1 })
)
expected = %(<select id="post_category" name="post[category]" required="required" size="1"><option value="" label=" "></option>\n<option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>)
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), {}, { required: true, size: 1 }))
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), required: true, size: 1))
end
def test_required_select_with_display_size_bigger_than_one
assert_dom_equal(
%(<select id="post_category" name="post[category]" required="required" size="2"><option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>),
select("post", "category", %w(abe mus hest), {}, { required: true, size: 2 })
)
expected = %(<select id="post_category" name="post[category]" required="required" size="2"><option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>)
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), {}, { required: true, size: 2 }))
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), required: true, size: 2))
end
def test_required_select_with_multiple_option
assert_dom_equal(
%(<input name="post[category][]" type="hidden" value="" autocomplete="off"/><select id="post_category" multiple="multiple" name="post[category][]" required="required"><option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>),
select("post", "category", %w(abe mus hest), {}, { required: true, multiple: true })
)
expected = %(<input name="post[category][]" type="hidden" value="" autocomplete="off"/><select id="post_category" multiple="multiple" name="post[category][]" required="required"><option value="abe">abe</option>\n<option value="mus">mus</option>\n<option value="hest">hest</option></select>)
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), {}, { required: true, multiple: true }))
assert_dom_equal(expected, select("post", "category", %w(abe mus hest), required: true, multiple: true))
end
def test_select_with_integer