Skip to content
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

Add multiple selections mode #106

Merged
merged 32 commits into from
Mar 20, 2024
Merged

Add multiple selections mode #106

merged 32 commits into from
Mar 20, 2024

Conversation

jlw
Copy link
Contributor

@jlw jlw commented Mar 13, 2024

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).

  • render selection "pills" in the "field" for clarity and compactness
  • stay open on selection
    • hide selected options from the listbox (and from keyboard navigation)
  • tweak keyboard navigation to work correctly in both single and multiple modes
    • use aria-current instead of aria-selected for multiple mode comboboxes

selections display
keyboard nav display
wrapping for many selections

Caveats:

  • I'm still pretty new to writing non-trivial Stimulus logic, so I'm sure there's a bunch of code smells here.
  • I haven't tested this with async.
  • I haven't tested this with the ability to add new options.
  • I thought I need Option#content to be public to display current selections on the initial render, but I probably missed another (better) alternative.
  • There are a couple of places here where I am checking 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.

@josefarias
Copy link
Owner

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.

@jlw
Copy link
Contributor Author

jlw commented Mar 14, 2024

@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.

@josefarias
Copy link
Owner

@jlw no worries! Go right ahead 👍

@josefarias
Copy link
Owner

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.

- render selection "pills" in the "field" for clarity and compactness
    - make `Option#content` public for initial render
- stay open on selection
    - hide selected options from listbox (and keyboard nav)
- tweak keyboard nav. to work correctly in single and multiple modes
    - use `aria-current` instead of `aria-selected` for multiple mode
@josefarias
Copy link
Owner

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.

@jlw
Copy link
Contributor Author

jlw commented Mar 15, 2024

@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.

@josefarias
Copy link
Owner

josefarias commented Mar 15, 2024

@jlw there are a couple things I'd like to rework about this PR today:

  • Big picture, I'm thinking a multiple-select combobox should be a regular combobox that does something special upon selection — it adds a chip to the input (and possibly keeps the listbox open, per your last comment, will revisit) EDIT: oh, and hides the already selected options from the listbox.
  • For that reason, I'd like to try and get this closer to "just do a regular selection... and then do this other thing" rather than "instead of a regular selection, do this special kind of selection". Designing for the first case should (hopefully) allow us to keep most of the inside workings the same without much need for conditionals throughout. But we'll see.
  • I'd like the chips to be added server-side. This will probably require mounting the engine for a default chip template (which I love your design for these, will definitely keep). But users should have the flexibility to render their own chips via something like :render_in
  • We need to make the chips keyboard-navigable. Something like this: https://www.w3.org/WAI/ARIA/apg/patterns/grid/examples/layout-grids/

This is great initial work and I'm happy to keep working on top of this so we can reach the final solution together.

@josefarias
Copy link
Owner

@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.

@josefarias
Copy link
Owner

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.

@josefarias
Copy link
Owner

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.

@josefarias
Copy link
Owner

josefarias commented Mar 19, 2024

We're almost ready to go!

Here's what's practically finished and tested (pending clean up):

  • Basic multiselect
  • Async multiselect
  • Multiselect with custom html in options and chips
  • Auto-dismissing multiselect (closed upon selection)
  • Prefilled multiselect
  • Form builder multiselect
  • A11y (using keyboard to navigate chips, and announcing selections/removals to the screenreader) – missing tests

Here's what I'm planning to do before calling this done:

  • Figure out how new values should work — currently thinking we only allow new values when name_when_new matches the original name. I don't think it would make sense otherwise, so I think we could just raise with a helpful message.
  • Test custom events (unrelated sidenote: might wanna rename these soon). @jlw already wrote a test, just gotta make sure it works with the newest stuff.
  • Test navigating chips with the keyboard, and if possible, test screenreader announcements.
  • Clean up
  • Merge!

@josefarias
Copy link
Owner

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.

@josefarias josefarias merged commit 659767e into josefarias:main Mar 20, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants