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

Combobox does not show results with multiple #2963

Closed
MrVauxs opened this issue Nov 14, 2024 · 9 comments
Closed

Combobox does not show results with multiple #2963

MrVauxs opened this issue Nov 14, 2024 · 9 comments
Assignees
Labels
feature request Request a feature or introduce and update to the project.
Milestone

Comments

@MrVauxs
Copy link

MrVauxs commented Nov 14, 2024

Current Behavior

Combobox does not fill the input with the selected choices if multiple is enabled true.

This also inadvertently causes required to always cause the input to fail the check.

Expected Behavior

Combobox filling the input with the given array, would be nice to be able to provide a function for the separator used (so you can have something like x, y, and z instead of plain .join(", ").

Steps To Reproduce

No response

Link to Reproduction / Stackblitz

No response

More Information

Earlier Discord Report for History: https://discord.com/channels/1003691521280856084/1191790107867488316/1303464712981184584

@MrVauxs MrVauxs added the bug Something isn't working label Nov 14, 2024
@endigo9740
Copy link
Contributor

FYI I noticed this too when testing the feature originally. It's always possible I was doing something wrong. But if not, then that indicates it may be a bug on Zag's end and would need to be reported upstream. Let's review first to confirm, but just a heads up!

@MrVauxs
Copy link
Author

MrVauxs commented Nov 15, 2024

FYI I noticed this too when testing the feature originally. It's always possible I was doing something wrong. But if not, then that indicates it may be a bug on Zag's end and would need to be reported upstream. Let's review first to confirm, but just a heads up!

I checked, the official React Zag StackBlitz instance works with multiple once you add it to the controls.
https://zagjs.com/components/react/combobox
https://stackblitz.com/run?file=src%2FApp.tsx,src%2FCombobox.tsx&showSidebar=0

@endigo9740
Copy link
Contributor

@MrVauxs can you expand on what you mean by "controls". Unfortunately your Stackblitz is non-functional for me. Just links to their homepage.

@MrVauxs
Copy link
Author

MrVauxs commented Nov 15, 2024

@MrVauxs can you expand on what you mean by "controls". Unfortunately your Stackblitz is non-functional for me. Just links to their homepage.

Oh, the stackblitz is supposed to be what is linked on the Zag page.

And what I meant is that the StackBlitz example Zag has, just doesnt have multiple (amongst many other stuff) as an available ComboboxProps property.

@endigo9740
Copy link
Contributor

endigo9740 commented Nov 15, 2024

I'm still completely lost here. multiple is a feature that Zag supports. Here it is in their documentation:
https://zagjs.com/components/react/combobox#selecting-multiple-values

But when implemented on our component, it's not working. Which is why this ticket was all about, correct? Maybe put Stackblitz aside and share whatever code you're referring to here please.

@MrVauxs
Copy link
Author

MrVauxs commented Nov 15, 2024

FYI I noticed this too when testing the feature originally. It's always possible I was doing something wrong. But if not, then that indicates it may be a bug on Zag's end and would need to be reported upstream. Let's review first to confirm, but just a heads up!

You weren't sure if this was a bug in Skeleton, or a bug in Zag. I was confirming that it is Skeletons, with an added comment about Zag's official example having outdated types but nonetheless working when you add the multiple property to their component.

@endigo9740 endigo9740 added this to the v3.0 (Next) milestone Nov 22, 2024
@phamduylong
Copy link
Contributor

I'll take a look at ZagJs's StackBlitz to see how they got the behavior working.

@endigo9740
Copy link
Contributor

endigo9740 commented Dec 9, 2024

@MrVauxs @phamduylong so two things with this...

First, the feature was filed as a bug ticket - which is fine. But does not adequately summarize the changes required. In short, a bug would imply we implemented something incorrectly or it's not behaving as expected. However this is not the case. The feature is working as expected, but the ability to support multiple would require additional effort and a redesign of the UI to support this. As such, I'm adjusting the label from bug -> feature request.

Second, as a feature request, it's at our discretion if we decide to support the request as proposed. And while there's certainly a use-case for a multiple-selection combobox type interface, I feel like it overlaps with another primitive we already provide - the Tags Input. So in order to support multiple-selection in the Combobox this would require essentially recreating the Tags Input within the Combobox component. I think this is beyond the scope of what we intend for these temporary component features - so I'm going to deny this for now.

Note that Long made a great attempt at supporting this in his PR, but you can see my notes here as to why I don't agree with the way this was implemented: #3008 (comment)

What I'm going to do instead is update the component and documentation to remove the multiple feature. I know it's not going to be the preferred solution, but this type of interface WILL be supported in the future when we have either:

  1. Floating UI Svelte completed and available
  2. The native Popover and CSS Anchoring APIs available cross browser

So other words, we will support something like this. Just not yet. If you would like me to clarify on this, let me know.

@endigo9740 endigo9740 added feature request Request a feature or introduce and update to the project. and removed bug Something isn't working labels Dec 9, 2024
@endigo9740
Copy link
Contributor

You can see the change here. This will be part of the next release: #3033

Again, if I can provide more context around the long term plans here, let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request a feature or introduce and update to the project.
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

3 participants