search

radix-ui / primitives

Radix Primitives is an open-source UI component library for building high-quality, accessible design systems and web apps. Maintained by @workos.
https://radix-ui.com/primitives
MIT License
15.44k stars 775 forks source link

`Select` Primitive Grouping Selection for Identical Values #2581

Closed shellscape closed 6 months ago

shellscape commented 9 months ago

Bug report

Current Behavior

When multiple items of the same value are added to a Select.Group, their labels from the associated ItemText components will be merged and displayed side-by-side in the Trigger. Checkbox indicators for both items will be displayed as well.

A few screenshots of the resulting UI

Screenshot 2023-12-10 at 5 04 22 PM Screenshot 2023-12-10 at 5 04 35 PM

Expected behavior

I would expect only one item, based on the label (e.g. name) to be displayed as "selected." That is how the native <select> works as can be seen here https://developer.mozilla.org/en-US/docs/Web/HTML/Element/select by changing all of the <option> to have the same value.

Reproducible example

Please accept my apologies for not creating a reproduction at this time. I'm a maintainer and routinely nuke issues from orbit that ignore the repro requirements. I probably won't have time to put together something that reproduces the issue for a while yet, but I wanted to get this tracked. I'm not a FE dev and move a lot slower with this stuff, was simply dabbling in some rare free time and ran into this issue.

Suggested solution

Merging labels and selection based on value equality, if intended, is unintuitive as a default behavior. At the least I would recommend a setting to prevent that. If the component internally is having trouble differentiating, then an internal unique ID or additional data attribute is likely needed.

Additional context

Your environment

Software Name(s) Version
Radix Package(s) @radix-ui/react-select 2.0.0
React n/a 18.12.0
Browser any
Assistive tech none
Node n/a v18
npm/yarn pnpm or bust!
Operating System macOS
benoitgrelard commented 6 months ago

values need to be unique, similar to a native select.

shellscape commented 6 months ago

@benoitgrelard unceremoniously closing an issue after letting it sit in limbo after 4 months, before you've engaged with the author, is pretty damn poor project management.

Merging labels and selection based on value equality, if intended, is unintuitive as a default behavior. At the least I would recommend a setting to prevent that. If the component internally is having trouble differentiating, then an internal unique ID or additional data attribute is likely needed.

This is still a valid suggested solution and would improve the DX of the component. Warnings for this scenario based on what the intended behavior you've described is, would also improve the DX. Throwing the ball over the fence with "this is what others do," when what others do is also sub-optimal, is lazy.

I would like to think that the DX of this library matters. why not solicit additional suggestions improvements before closing an issue and marking it as expected? I spent some time in gathering information and screenshots to present the issue, and how you've dealt with it shows absolutely no appreciation for that.

benoitgrelard commented 6 months ago

Sorry @shellscape I could have been more explicit.

I'm a maintainer and routinely nuke issues from orbit that ignore the repro requirements.

We get a very high volume of issues, a lot of them just being questions, etc so it's not easy to keep on top of.

I have been going through hundreds of issues on the repo in the past few days and closed almost a hundred issues, a lot of them being very poor, so I know precisely what you mean.

I was probably reaching saturation point when I got to yours… Anyway, I apologise for not going deeper on this one.

Given the current complexity of the Select architecture, I don't think it would be easy or even possible (or even make sense) to try and accommodate for duplicate values.

If you wire a controlled native select in React with options with duplicate values, you'll see similar behaviour too (ie, it will only show the first matching option as selected).

Granted, the failure in our case looks perhaps more confusing, although one could argue it's more obvious that you've done something wrong and clearer to understand what it is you did wrong.

FWIW, it is very broadly understood that values need to be unique in such components, same for radio groups, etc, otherwise the components have no way to determine what to match.

It's always a difficult line to thread for a library like ours in terms of shipping extra defensive code for things like these, that are mostly obvious, yet, could be nice DX for more junior developers.

The best we could do here I think would be to throw an error if we "see" the same value more than once, I will look into it to see if this can be done without too much extra complexity.