Skip to content

Improves select #33

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

Open
nolantait wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Improves select #33

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be16111
Adds multiple and include blank options to select
nolantait Apr 2, 2024
e8c97e2
Adds overriding select name if multiple
nolantait Apr 2, 2024
155824e
Adds multiple to select html attributes
nolantait Apr 2, 2024
8a04f5a
Moves multiselect field to its own component
nolantait Apr 28, 2024
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
27 changes: 27 additions & 0 deletions lib/superform/rails.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ def select(*collection, **attributes, &)
Components::SelectField.new(self, attributes: attributes, collection: collection, &)
end

def multiple_select(*collection, **attributes, &)
Components::MultipleSelectField.new(self, attributes: attributes, collection: collection, &)
end

def title
key.to_s.titleize
end
Expand Down Expand Up @@ -331,10 +335,33 @@ def false_option(&)
end

protected

def map_options(collection)
OptionMapper.new(collection)
end
end

class MultipleSelectField < SelectField
def template(...)
# The HTML specification says when multiple parameter passed to select
# and all options got deselected web browsers do not send any value to
# server. Unfortunately this introduces a gotcha: if a User model has
# many roles and have role_ids accessor, and in the form that edits
# roles of the user the user deselects all roles from role_ids
# multiple select box, no role_ids parameter is sent.
input(type: "hidden", name: dom.name, value: "")
super(...)
Copy link

@mvkampen mvkampen Apr 23, 2025

Choose a reason for hiding this comment

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

I also used the argument forwarding, but i realized that this was introduced in ruby 2.7 so then it needs to bump required_ruby_version in the gemspec. If you don't want to include that in this ticket then switch to *, **, & to catch all arguments and can forward explicit, or implicitly super without parenthesis. For these discussions we could include more code style guidance (e.g. rubocop)?

end

protected

def field_attributes
super.merge(
name: "#{dom.name}[]",
Copy link
Contributor

@bradgessler bradgessler Apr 29, 2024

Choose a reason for hiding this comment

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

The field(:foo).collection method will generate the correct field names for this without having to manually interpolate strings into the field name. Example at https://github.com/rubymonolith/superform/blob/main/README.md?plain=1#L135-L142 and implementation at https://github.com/rubymonolith/superform/blob/main/lib/superform.rb#L224-L259

I haven't thought too much about exactly how this would be integrated, but if you're willing to bounce around a bit with these commits I think we can get there.

Copy link

@mvkampen mvkampen Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do we prefer field.select(multiple: true) over field.multiple_select() i know i do.

The 2nd link is broken, do you mean the
FieldCollection class?

Not entirely sure what you mean on how to use the collection I guess starting with radiobutton would be slightly easier as you don't have to deal with this particular issue about key naming as it is of singular value. As your suggestion here #13

Intuitively it is easier to have the element be responsible for both key and value 1 to 1.
Whereas here you have field(:fruits).collection to yield each <option> but its key should be used in the parent <select> element. However you could argue that select itself should be responsible to generate its key, and not require us to generate collection internally to retrieve the name?
Here the reference to rails key naming for multiple values in form_tag_helper

The code affecting value keys naming in superform.
One more issue that this PR seems to miss is rendering selected options
field.value == key in this case value would be an array and then include? would be the right predicate?

<select name="fruits[]" multiple>
  <option value="apple" selected>Apple</option>
  <option value="banana">Banana</option>
  <option value="cherry" selected>Cherry</option>
</select>

<input type="checkbox" name="fruits[]" value="Apple">
<input type="checkbox" name="fruits[]" value="Banana">
<input type="checkbox" name="fruits[]" value="Cherry">

<input type="radio" name="fruit" value="Apple">
<input type="radio" name="fruit" value="Banana">
<input type="radio" name="fruit" value="Cherry">

multiple: true
)
end
end
end
end
end