-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Make the Combobox
component nullable
by default
#3064
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
+ cleanup and adjust comments
This gives us `NoInfer<T>`!
Now that `nullable` is gone, we can take another look at the type definition. This in combination with the new `NoInfer` type makes types drastically simpler and more correct.
But let's mark it as deprecated to hint that something changed.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
If we are just checking for `T extends null`, then `{id:1,name:string}|null` will also be true and therefore we would eventually return `string` instead of `"id" | "name"`. To solve this, we first check if `NonNullable<T> extends never`, this would be the case if `T` is `null`. Otherwise, we know it's not just `null` but it can be something else with or without `null`. To be sure, we use `keyof NonNullable<null>` to get rid of the `null` part and to only keep the rest of the object (if it's an object).
This way the `by` prop will still compare single values that are present inside the array. This now also solves a pending TypeScript issue that we used to `// @ts-expect-error` before.
We have some tests that use uncontrolled components which means that we can't infer the type from the `value` type.
Now that we don't infer the type when using the generic inside of `onChange`, it means that we can use `onChange={setValue}` directly because we don't have to worry about the updater function of `setValue` anymore.
If you are in single value mode, then the `onChange` can (and will) receive `null` as a value (when you clear the input field). We never properly typed it so this fixes that. In multiple value mode this won't happen, if anything the value will be `[]` but not `null`.
thecrypticace
requested changes
Mar 29, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general — just a small note about the tests.
thecrypticace
approved these changes
Mar 29, 2024
Hi @RobinMalfait. Is this change coming to vue as well? |
This was referenced May 28, 2024
corneliusroemer
added a commit
to loculus-project/loculus
that referenced
this pull request
Jun 29, 2024
corneliusroemer
added a commit
to loculus-project/loculus
that referenced
this pull request
Jun 29, 2024
…able combobox Headlessui v2 makes it necessary to handle null input for combobox. See tailwindlabs/headlessui#3064
corneliusroemer
added a commit
to loculus-project/loculus
that referenced
this pull request
Jun 29, 2024
corneliusroemer
added a commit
to loculus-project/loculus
that referenced
this pull request
Jun 29, 2024
…able combobox Headlessui v2 makes it necessary to handle null input for combobox. See tailwindlabs/headlessui#3064
corneliusroemer
added a commit
to loculus-project/loculus
that referenced
this pull request
Jun 29, 2024
corneliusroemer
added a commit
to loculus-project/loculus
that referenced
this pull request
Jun 29, 2024
…able combobox Headlessui v2 makes it necessary to handle null input for combobox. See tailwindlabs/headlessui#3064
corneliusroemer
added a commit
to loculus-project/loculus
that referenced
this pull request
Jun 30, 2024
…2200) * chore(deps): bump @headlessui/react from 1.7.19 to 2.1.1 in /website Bumps [@headlessui/react](https://github.com/tailwindlabs/headlessui/tree/HEAD/packages/@headlessui-react) from 1.7.19 to 2.1.1. - [Release notes](https://github.com/tailwindlabs/headlessui/releases) - [Changelog](https://github.com/tailwindlabs/headlessui/blob/main/packages/@headlessui-react/CHANGELOG.md) - [Commits](https://github.com/tailwindlabs/headlessui/commits/@headlessui/react@v2.1.1/packages/@headlessui-react) --- updated-dependencies: - dependency-name: "@headlessui/react" dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * chore(website): migrate headless ui to use DialogPanel instead of Dialog.Overlay See https://github.com/tailwindlabs/headlessui/releases/tag/%40headlessui%2Freact%40v1.6.0 for migration guide * chore(website): address Combobox nullable breaking change for headless-ui v2 see tailwindlabs/headlessui#3064 * chore(website): appease typescript for headlessui v2 upgrade for nullable combobox Headlessui v2 makes it necessary to handle null input for combobox. See tailwindlabs/headlessui#3064 * chore(website): address headlessui v2 deprecation warnings due to renamings * chore(website): use focus instead of deprecated active, headlessui v2 migration see https://github.com/tailwindlabs/headlessui/releases/tag/%40headlessui%2Freact%40v2.0.0#user-content-upgrading-from-v1 * chore(website): mock ResizeObserver, neccesary for headlessui v2 see tailwindlabs/headlessui#3268 * Use new `immediate` feature of headlessui v2 * Fix flaky test by waiting for modal to be removed * fix failing test by using userEvent.click instead of fireEvent.click * chore: address warnings/lints --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Cornelius Roemer <cornelius.roemer@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes the
Combobox
nullable by default, this means that it's possible to receive anull
value in theonChange
callback. The types have been updated to tell that theonChange
can receivenull
. Note: this is only in single value mode.This PR also improves the behaviour of the component in general.
If you remove all the text from the
ComboboxInput
, you will receive anull
value in theonChange
callback.If you click outside of the
Combobox
, theCombobox
will close and then a few things can happen:Combobox
didn't have a value, so it will remain empty.Combobox
had a value, so it will keep that value that was previously selected.It will not select the currently
active
value (active means the visually selected value without being selected in theCombobox
state itself).Think of it as a "cancel" action.
If you press
Escape
, theCombobox
will close and the same rules as above will apply. Think of it as a "cancel" action.If you press
<tab>
, theCombobox
will close but this time theactive
value will beselected
.This PR also improves the underlying types, and uses the new TypeScript 5.4
NoInfer<T>
feature. This way we can cleanup some complex types and make the code more readable.This is a breaking change, but you can apply the following changes to your component:
Scenario #1: If you were already using
nullable
:In this scenario, you should be able to just drop the
nullable
prop. We currently still have it in the TypeScript types because it doesn't hurt, it just won't do anything since it's the default now.<Combobox value={value} onChange={setValue} - nullable > <Combobox.Input onChange={(e) => setQuery(e.target.value)} /> <Combobox.Options> <Combobox.Option value="a">Option A</Combobox.Option> <Combobox.Option value="b">Option B</Combobox.Option> <Combobox.Option value="c">Option C</Combobox.Option> </Combobox.Options> </Combobox>
Scenario #2: If you explicitly don't want it to be nullable:
In this case, you can check the value in the
onChange
, and apply the necessary logic that you want. You could set the value to some specific (fallback) value, you can abort a form submission, anything that fits your current domain logic.