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

fix: Select's onChange sends undefined if current value is selected #412

Open
lautaropaske opened this issue May 15, 2024 · 2 comments
Open

Comments

@lautaropaske
Copy link

lautaropaske commented May 15, 2024

Describe the bug
Implementing the Select primitive I found that the onChange function passes undefined as an argument to the callback if the current value selected is reselected. This breaks the type declared for the requested callback. In other terms: the function behaves as onChange?: (value: T | undefined) => void;.

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://kobalte.dev/docs/core/components/select
  2. At the demo, click on the Select component
  3. Select the currently selected value (i.e. Blueberry)
  4. See error: value is lost in the select content

Expected behavior
If the current value is selected, the onChange function should be called with it as a value or it shouldn't be called. Test the same flow in https://www.radix-ui.com/primitives/docs/components/select, it doesn't affect the current value selected

Desktop:

  • OS: macOS
  • Browser Arc
  • Version Chromium Engine Version 124

Additional context
None

@jer3m01
Copy link
Member

jer3m01 commented May 15, 2024

By default Kobalte allows you to unselect an option (by clicking on the current option) and so send undefined to onChange. You're correct that the type doesn't match, I'll update it to include undefined.

If you wish to have the same behavior as radix set disallowEmptySelection on your Select (Root).

@jcmonnin
Copy link

I fear the issue is slightly more complicated. Unfortunately, onChange gives null instead of undefined when unselecting, which is problematic for user's code. I still apply a workaround to convert null to undefined because of that. It would be good to give undefined instead of null.

Note that for multi select, it's probably fine if value cannot be undefined since supplying empty array when nothing is selected is probably better. These are the function signatures I would expect:

  • Single Select onChange: (value?: T) => void
  • Multi Select: onChange: (value: T[]) => void

Note that this is linked to #252

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋 To do
Development

No branches or pull requests

3 participants