-
Notifications
You must be signed in to change notification settings - Fork 25
Checkbox - allow collections, add Radio component #65
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
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 is heading in a good direction. There's a few things I noted in the source that get those closer towards me merging it into main. Most of the issues are straight forward; however the one that might need more discussion is how to handle the collection of radio buttons and labels for inputs when not calling via a block.
README.md
Outdated
| Field(:plan).label { "Choose your plan" } | ||
| # Radio group with options - renders multiple radio buttons | ||
| Field(:plan).radio( | ||
| options: [ |
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 options should look like the select field where they're passed in as positional arguments only. It can accept these arguments:
-
Field(:color).radio("Red", "Blue", "Green") would create the radio buttons with the labels and the ID that converts the value to an ID, probablyred,blue,green`. -
Field(:color).radio([1, "Red"], [2, "Blue"], [3, "Green"])where id is first and the label is second and -
Field(:color).radio Color.select(:id, :name)
When people want to work directly with options, they should do so via blocks.
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.
Happy to get rid of the options kwarg path and restrict these to being passed as positional args.
However, I wonder if you recall Select is written to take options as a kwarg, or positional args.
- Because of the way the constructor is written, options can be sent under the kwarg "collection". This feature is undocumented in the README - maybe nobody uses it, and it's effectively dead code? I'm refactoring checkbox and radio to get rid of the constructor arg.
- I would propose getting rid of that in
select, or at least deprecating it. If we want to keep it, IMO it could be renamedoptions, because it is an array of values for<option>elements. This use of the term "collection" (in the definition ofSelect) seems incongruous with its use elsewhere in Superform. (Am i right about that?)
lib/superform/rails/field.rb
Outdated
|
|
||
| def radio(value, *, **, &) | ||
| input(*, **, type: :radio, value: value, &) | ||
| def radio(*args, **attributes, &) |
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.
Eliminating options can simplify this method to something like
if args.any? or if block_given?
# Call up to Components::Radio
else
# Use the input tag
endYou don't need to fix this, but I'm realizing in the rest of my code I'm passing blocks into input, which is a void element.
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 can do a separate preliminary PR re: input and blocks while i'm at it.
|
|
||
| def buttons(*option_list) | ||
| map_options(option_list).each do |value, label| | ||
| button(value) { label } |
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 is invalid HTML, input tags can't have content inside of them because they're void elements. What you can do instead is something like:
<label>Red<input type="radio" value="red" id=""></label>I think that's fine for when people only pass positional arguments and nothing else, but this approach might be too basic though because when people work with radio buttons in blocks.
They should be able to do something like this:
Field(:color).radio(Color.select(:id, :name)).buttons.each do
div(class: "flex flex-row"){
it.label(class: "font-bold bg-red-200")
div { "Let's put something here just to be annoying" }
it.button
end
endI haven't fully thought through the buttons method above since there's no other fields like it. Maybe a better approach would be something like:
Field(:color).radio_buttons(Color.select(:id, :name)).each do
div(class: "flex flex-row"){
it.label(class: "font-bold bg-red-200")
div { "Let's put something here just to be annoying" }
it.button
end
endWhich avoids a conflict with the radio method entirely. This would be true for checkboxes too, which haven't been implemented yet.
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.
as documented now in the README or the PR summary above...
Current API on this branch - you can either use the current single checkbox call field(:foo).checkbox, or you can pass a collection as positional args to field(:foo).checkbox(*opts). To me this is preferable to having a second method like checkbox_buttons. The generated "checkbox-grouping" names and IDs will be correctly generated in the HTML.
Ditto for radio - i don't think we need a radio_buttons method since radios are always displayed as a group of options.
|
For now i'm adding It also might make sense to add the |
In collection mode, the `button` method explicitly sets all input
attributes (type, id, name, value, checked), making the `name` attribute
from `field_attributes` redundant.
Changed `field_attributes` to return `{}` in collection mode since the
`button` method handles all attributes explicitly.
Updated test expectations to match new attribute order (functionally
identical HTML, just different order: type, id, name, value, checked).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
|
To sum up recent changes on this PR:
My goal has been to keep the APIs for both of these methods, plus |
This PR adds array mode to Checkbox and adds Radio components, enabling multi-select checkboxes and radio button groups.
What's New
optionmethodoptionmethod for rendering custom content, asSelectcomponent currently doesBackward Compatibility
All existing boolean checkbox functionality remains unchanged.
New API
Checkbox Array Mode
Multi-select with positional arguments
Custom rendering with block
With ActiveRecord relations
Boolean mode (unchanged)
Radio Array Mode
Single-select with positional arguments
Custom rendering with block
With ActiveRecord relations
Usage Examples
Basic Form Example
Advanced: Custom Markup
Option Format
Options accept multiple formats for flexibility:
Component API
Checkbox Component
Radio Component
Implementation Details
How Array Mode is Determined
Checkbox:
Radio:
Checked State Logic
Checkbox (multi-select):
Radio (single-select):
HTML Output
Checkbox array:
Radio group:
Why This Design?
Positional Arguments
Options are inherently a list of choices. Passing them as positional arguments (
*args) is more natural than a keyword argument:Block-Only Support
Enables clean custom rendering without redundant constructor arguments:
Consistent Naming
Using
optionaligns with Select component terminology and HTML<option>elements: