-
Notifications
You must be signed in to change notification settings - Fork 25
Add multiple keyword/functionality to select
#64
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?
Conversation
multiple and include_blank keywords to select
|
more details: Multiple select support# Multiple select with automatic [] appending and hidden input
field(:tag_ids).select(
[1, "Ruby"], [2, "Rails"], [3, "Phlex"],
multiple: true
)
# Renders: <input type="hidden" name="tag_ids[]" value="">
# <select name="tag_ids[]" multiple>...</select>Include blank option# Add blank option at the start
field(:country).select(
nil, [1, "USA"], [2, "Canada"], [3, "Mexico"]
)API harmonized with with radio component PR #65# Positional arguments
field(:role_id).select([1, 'Admin'], [2, 'Editor'], [3, 'Viewer'])
# ActiveRecord relations (wrapped in array)
field(:author_id).select(User.select(:id, :name))NotesMultiple select handling
Field naming logic# Single select
name="role_id"
# Multiple select
name="role_ids[]"
# Multiple select in collection
name="user[orders][0][tag_ids][]"
# Multiple select in Field collection (parent is Field)
name="users[][]" # No extra [] appendedForm integration testsIntegration tests to confirm Select works correctly in Rails forms: Single Select
Multiple Select
|
|
Feedback is similar to the radio buttons PR Eliminate the Eliminate the Similar to |
|
PR updated to reflect feedback. Question (also asked on #65, but relevant here) In Select currently, the # undocumented kwarg on main
field(:foo).select(collection: [[1, 'Admin'], [2, 'Editor']])
# positional
field(:foo).select([1, 'Admin'], [2, 'Editor'])Is this kwarg available just because of the way it's initialized? Or is this considered a feature undocumented in the README? Maybe nobody uses it, can add a deprecation notice if passed directly. |
multiple and include_blank keywords to selectmultiple keyword/functionality to select
An unexpected API gotcha i'm sure you're aware of:Superform This is a real gotcha for refactoring from ERB, it means devs have to manually flip all the arrays. It would be good to change this API. I can't imagine how to change it without breaking people's existing |
bradgessler
left a comment
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.
This commit feels "too big" for the functionality it implements. Changes to the README, 500+ LOC specs, and the Ruby version change feels like an LLM got carried away.
As far as the actual code goes, I left comments inline for a few things to change.
README.md
Outdated
| Field(:body).textarea(rows: 10) | ||
| Field(:publish_at).date | ||
| Field(:featured).checkbox | ||
| field(:title).text |
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.
These Field methods should remain capitalized.
| # Hidden input ensures a value is sent even when all options are | ||
| # deselected in a multiple select | ||
| if @multiple | ||
| hidden_name = field.parent.is_a?(Superform::Field) ? dom.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.
I think I use "#{dom.name}[]" in a collection somewhere. Could that same class be used here? If it doesn't make sense to drop that class in here then "#{dom.name}[]" should be dropped into a helper class method somewhere and called so it goes through the same code path.
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 see what you mean. This is something we probably want to be reusable, in light of the checkbox/radio group PR coming up.
Does it make sense to add a method to the DOM class called array_name?
# Returns the name with `[]` appended for array/multiple value fields.
# Used by multiple selects, checkbox groups, etc.
def array_name
"#{name}[]"
end
lib/superform/rails/field.rb
Outdated
| Components::Select.new(field, attributes:, collection:, &) | ||
| def select(*options, **attributes, &) | ||
| # Extract select-specific parameters from attributes | ||
| multiple = attributes.delete(:multiple) || false |
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.
Move into the method definition: def select(*options, multiple: false, **attributes, &)
No need to extract.
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.
Understood - done.
superform.gemspec
Outdated
| spec.homepage = "https://github.com/rubymonolith/superform" | ||
| spec.license = "MIT" | ||
| spec.required_ruby_version = ">= 2.6.0" | ||
| spec.required_ruby_version = ">= 2.7.0" |
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.
This should work with Ruby 2.6. If it doesn't, bump Ruby versions in different commits.
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.
Moving this change to a new PR branch.
| map_options(collection).each do |key, value| | ||
| option(selected: field.value == key, value: key) { value } | ||
| # Handle both single values and arrays (for multiple selects) | ||
| selected = Array(field.value).include?(key) |
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.
This allocates a new array per iteration. Move selected outside of the loop if it doesn't need to be there and then check if the key is included in selected.
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.
Done
| select(**attributes) { options(*@collection) } | ||
| select(**attributes) do | ||
| # If first option is nil, render a blank option | ||
| include_blank = @options.first.nil? |
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'm surprised to see this. What if blank is the last option? Or first? It could also be 3rd, 5th, and 8th.
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.
Understood. Adjusted to handle nil at any position
|
That's all true about the LLM, but
|
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Avoids allocating a new array on each iteration when checking if a value is selected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Previously nil was only handled as the first option. Now nil renders as a blank option wherever it appears in the collection. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Cleaner than extracting from attributes hash. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The capitalized Field is intentional - it's the Phlex Kit syntax. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The capitalized Field is intentional - it's the Phlex Kit syntax. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…erform into nimmo-improve-select
This is a reboot of PR #33. It intends to support the
multipleHTML spec for<select>elements.multiplekwarg to theselectmethod. Full backwards compatibility with the current API ofselect.select multiplefield correctly — i.e., using square brackets in a way that does not interfere with parent field notation. (It has to prevent double[]when multiple select is used inside collections.) See this conversation. Tests all permutations I could think of.multiple, to handle the way browsers handle blank selects (form value may not be submitted unless hidden field present)Selectcomponent param for the array of options fromcollectiontooptions(to avoid confusion with.collectionmethod, which creates FieldCollections). Backwards compatible, both keywords accepted.fieldvsField)Updates the GEMSPEC to require Ruby 2.7, since the Superform codebase already uses anonymous arg forwarding all over the place (so it already effectively requires 2.7).Note, I realize you proposed a separate
multiple_selectcomponent method [in the discussion here] (#33 (comment)) and mentioned that also below, but I would suggest this is not really necessary or desirable, since the current PR maintains backwards compatibility with the current method signature. Keeping theselectlogic in one component seems to be the most intuitive and elegant way. In theselectHTML element spec,multipleis just an attribute, not a separate element.