-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
base: main
Are you sure you want to change the base?
Improves select #33
Conversation
|
Thanks for the PR! I didn't realize there was a specification for I ran into a similar problem with checkboxes where it actually needs multiple HTML elements to deliver the desired behavior, which I solved via its own component: superform/lib/superform/rails.rb Lines 255 to 267 in 00f0562
Since I want Superform to do as little as possible to alter the constructs of HTML, I'd like to explore creating a component for multiple select that isolates this behavior in its own component instead of altering the Here's what that would mean to get this merged:
I'm avoiding weird Rails form helper constructs like |
|
|
||
| def field_attributes | ||
| super.merge( | ||
| name: "#{dom.name}[]", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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">| # 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(...) |
There was a problem hiding this comment.
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)?
This branch adds a few core features to select according to https://apidock.com/rails/ActionView/Helpers/FormOptionsHelper/select
include_blankkeyword that adds a blank optionmultiplekeyword that changesnameto add[]if mutliple is trueRelevant part from the link above: