Closed jlw closed 5 months ago
Phenomenal work here @jlw thanks for tackling such a tricky feature head-on. Kudos.
This is foundational work β and very tricky. So if you don't mind I'll jump in and make some changes myself (if I can remember how to modify someone else's PR) and we can collaborate asynchronously by communicating here.
@josefarias - this branch's history is showing up quite oddly in my working copy after your merge, so I'd like to force-push up a cleanly-rebased copy of this branch on top of these most recent changes I missed. I know that might cause a bit of headache on your end if you've started any real changes to this PR, so I'll wait for your thumbs-up before I proceed.
@jlw no worries! Go right ahead π
I plan to start working on this tomorrow. I've only glanced at things so first order of business is understanding your implementation fully.
Next will be evaluating the public API.
In case there are no changes there, then I'd probably look at some cleanup, aiming to remove some conditionals and such.
I'll write here every step of the way so you get a chance to share your thoughts.
I worked on some significant refactoring of the main codebase today. It's all merged now.
The changes resulted in some merge conflicts but once those are taken care of the rest of the feature should be much easier to work on now that everything's detangled.
Planning to start work on this in earnest tomorrow.
@josefarias - since you're evaluating this carefully for the public API, I should have mentioned an additional feature that I left out for initial simplicity. I can easily imagine wanting to switch whether the listbox stays open on selection or closes immediately. I have considered in my own app keeping the listbox open on create/edit forms and having it close on search/filter forms.
@jlw there are a couple things I'd like to rework about this PR today:
:render_in
This is great initial work and I'm happy to keep working on top of this so we can reach the final solution together.
@jlw made some good progress today.
I spiked on an approach that renders the chips on the server. That will allow users to extend that behavior and render their own chips as necessary by creating view templates.
I also made the chips navigable by keyboard. Specifically, using tab and backspace to focus and remove chips.
I'm feeling good about the feasibility of the "regular selection, and then something else" approach. But it's not finished. Chips are created and destroyed as necessary, but no value is persisted in the combobox state.
I'm thinking it should be feasible to support comma-separated values in the combobox. Then, when reading/writing to/from the value, we check if the combobox is multiselect and convert to/from an array as necessary. If we can do this then there's no need to implement specific behaviors for prefilled comboboxes, for example.
Finally, although this is not what's currently implemented, I'm leaning towards having the combobox close after adding a selection. I think leaving it open complicates the code too much to be worth it, but I haven't thought it through. Specifically, I'm worried about the user losing their place if we don't leave the combobox open exactly where they selected the latest option. As much as the "regular selection, and then something else" approach gains us, this is one area that can get complicated, because we now need to remember the last selection in-between selections. But who knows, we'll see where the code takes us.
For what it's worth, re-opening the combobox is trivial now that users can connect a stimulus controller after streaming in their selection chips.
More work left to do. I plan to keep at it tomorrow.
I've implemented the comma-separated values approach.
This solidified the multiselect aspect of the library as a sort of wrapper around the standard combobox. This is great because any modifications to the standard single-select behavior are carried over to multiselect comboboxes automatically.
My current mental model for multiselects is that it's a wrapping component that only cares when a selection is finalized in the standard combobox (a sub-component of the multiselect, if you will). When a selection is finalized, a chip is added, the selected option is hidden (contrary to what happens in single-selects), and the value is added to a list of comma-separated values, which is ultimately sent to the server upon form submission.
In reality it's all one component. I'm only describing my mental model to guide future implementation decisions.
After having played with it a while I really do like leaving the combobox open (albeit reset to a blank query). I don't think it will be too tricky after all. Implementing that next after implementing some preselection logic we will need after all.
We did end up with some conditionals in the new "regular selection, and then something else" approach. But they're all clustered together in a way that I think makes sense. Really liking the current direction.
We're almost ready to go!
Here's what's practically finished and tested (pending clean up):
Here's what I'm planning to do before calling this done:
Okay this is practically done, only missing some cleanup. Which this very much needs. But I wanna get this merged before @Deanout and I pair on grouped options tomorrow. Will clean up in a separate PR before release.
This is the most common use case for comboboxes in the internal app I'm building - we either want to filter our records down to a subset of values in a given column, or we want to assign multiple values to a record (this can handle both a
has_many
relationship as well as a legacy pattern I inherited with PostgreSQL text array columns).aria-current
instead ofaria-selected
for multiple mode comboboxesCaveats:
Option#content
to be public to display current selections on the initial render, but I probably missed another (better) alternative.if (!this.isMultiple()
- based on my manual testing some of those might be able to be removed, but I want to spend more time testing on that (I hope to open a tiny PR on this soon) and am out of time for the moment and want to start this conversation.