-
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
Improve UX by freezing <ComboboxOptions />
component while closing
#3304
Conversation
When the `Combobox` is in a closed state, but still visible (aka transitioning out), then we want to freeze the `children` of the `ComboboxOptions`. This way we still look at the old list while transitioning out and you can safely reset any `state` that filters the options in the `onClose` callback. Note: we want to only freeze the children of the `ComboboxOptions`, not the `ComboboxOptions` itself because we are still applying the necessary data attributes to make the transition happen. Similarly, if you are using the `virtual` prop, then we only freeze the `virtual.options` and render the _old_ list while transitioning out.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
// We should keep updating the frozen value, as long as we shouldn't freeze | ||
// the value yet. The moment we should freeze the value we stop updating it | ||
// which allows us to reference the "previous" (thus frozen) value. | ||
if (!freeze && frozenValue !== data) { | ||
setFrozenValue(data) | ||
} |
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.
This is nice because we don't use any effects, but it will trigger a setFrozenValue
every time the data
changes. Ideally we only change this state the moment we go from false
to true
and capture the old value somehow. 🤔
theirProps: { | ||
...theirProps, | ||
children: ( | ||
<Frozen freeze={visible && data.comboboxState === ComboboxState.Closed}> | ||
{typeof theirProps.children === 'function' | ||
? // @ts-expect-error The `children` prop now is a callback function | ||
theirProps.children?.(slot) | ||
: theirProps.children} | ||
</Frozen> | ||
), | ||
}, |
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.
Had to do this here to make sure we only freeze the children
, and not the ComboboxOptions
itself (aka, the return render()
)
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.
It feels a bit weird because it kinda feels like this should still be the responsibility of render (or a similar helper). But, having said that, this is fine for now.
// Map the children in a scrollable container when virtualization is enabled | ||
if (data.virtual && visible) { | ||
if (data.virtual) { | ||
if (options === undefined) throw new Error('Missing `options` in virtual mode') |
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.
Just to make TypeScript happy 😅
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.
Small tweaks is all but afaict it's working well. I did find a case where I hit an error — but not sure if related (maybe?):
- Go to http://localhost:3000/combobox/combobox-virtualized
- Open the first combobox
- Select all
- Delete
- Hit return
- Press
a
while it's fading out
packages/@headlessui-react/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/@headlessui-react/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
theirProps: { | ||
...theirProps, | ||
children: ( | ||
<Frozen freeze={visible && data.comboboxState === ComboboxState.Closed}> | ||
{typeof theirProps.children === 'function' | ||
? // @ts-expect-error The `children` prop now is a callback function | ||
theirProps.children?.(slot) | ||
: theirProps.children} | ||
</Frozen> | ||
), | ||
}, |
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.
It feels a bit weird because it kinda feels like this should still be the responsibility of render (or a similar helper). But, having said that, this is fine for now.
ComboboxOptions
<ComboboxOptions />
component while closing
This PR improves the UX when you are closing a
Combobox
that uses aTransition
while closing.Typically the
Combobox
component is used with a filtered list based on the value of theComboboxInput
. When theCombobox
closes then theonClose
will be called. In this function you typically reset the search query state as well:Notice that we use
setQuery('')
in theonClose
.If you are using a transition, then the following things will happen:
Combobox
is closedonClose
is triggered, theComboboxOptions
are transitioning outquery
is resetfilteredPeople
list is updated (not filtered yet)ComboboxOptions
re-renders with the "full" list instead of the filtered list.Transition
completes after this.This now means that while transitioning out, the
ComboboxOptions
is re-rendering with a new list of options even though you just selected an option.This PR now will "freeze" that state so that we show the contents at the time of closing the
Combobox
.