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 required double ArrowDown requirement #1281

Merged
merged 2 commits into from
Mar 29, 2022
Merged

Conversation

RobinMalfait
Copy link
Collaborator

@RobinMalfait RobinMalfait commented Mar 29, 2022

If the activeOptionIndex is set to null, then we default to the very first non-disabled option. This data is not stored in state because if you as the user go to a specific option, then start searching then we will maintain the active option. This means that we have to update the activeOptionIndex when options are moving around.

While making the first option the active one, we can't store that in state directly otherwise the very first option becomes the active one. If we then inject combobox options before the current one then all of a sudden your active option would jump around a bit.

We don't want this jumping to happen, we want the very first option to be the one that's active no matter which option it is.

Since this is not stored in state, our keydown handler was a bit borked.
Internally it thinks we are still at activeOptionIndex === null therefore pressing arrow down would move us to activeOptionIndex === 0. To go to the second option, we can press down again which would move us to activeOptionIndex === 1. The only issue is that visually we were already at 0.

This fixes that by making sure that if we have activeOptionIndex === null that we fallback to the very first non disabled option before we execute the goToOption() code.

Before (3 steps):

Open combobox, activeOptionIndex === null

Combobox
Option A (active)
Option B
Option C

Arrow Down, activeOptionIndex === 0 ! Uh oh

Combobox
Option A (active)
Option B
Option C

Arrow Down, activeOptionIndex === 1

Combobox
Option A
Option B (active)
Option C

After (2 steps):

Open combobox, activeOptionIndex === null

Combobox
Option A (active)
Option B
Option C

Arrow Down, activeOptionIndex === 1

Combobox
Option A
Option B (active)
Option C

If the `activeOptionIndex` is set to `null`, then we default to the very
first non-disabled option. This data is _not_ stored in state because if
you as the user go to a specific option, then start searching then we
will maintain the active option. This means that we have to **update**
the `activeOptionIndex` when options are moving around.

While making the first option the active one, we can't store that in
state directly otherwise the very first option becomes the active one.
If we then inject combobox options _before_ the current one then all of
a sudden your active option would jump around a bit.

We don't want this jumping to happen, we want the very first option to
be the one that's active no matter which option it is.

Since this is not stored in state, our keydown handler was a bit borked.
Internally it thinks we are still at `activeOptionIndex === null`
therefore pressing arrow down would move us to `activeOptionIndex ===
0`. To go to the second option, we can press down again which would move
us to `activeOptionIndex === 1`. The only issue is that visually we were
already at `0`.

This fixes that by making sure that if we have `activeOptionIndex ===
null` that we fallback to the very first non disabled option _before_ we
execute the `goToOption()` code.

 ### Before:

**Open combobox**, `activeOptionIndex === null`

| Combobox                |
| ----------------------- |
| **Option A** _(active)_ |
| Option B                |
| Option C                |

**Arrow Down**, `activeOptionIndex === 0`

| Combobox                |
| ----------------------- |
| **Option A** _(active)_ |
| Option B                |
| Option C                |

**Arrow Down**, `activeOptionIndex === 1`

| Combobox                |
| ----------------------- |
| Option A                |
| **Option B** _(active)_ |
| Option C                |

 ### After:

**Open combobox**, `activeOptionIndex === null`

| Combobox                |
| ----------------------- |
| **Option A** _(active)_ |
| Option B                |
| Option C                |

**Arrow Down**, `activeOptionIndex === 1`

| Combobox                |
| ----------------------- |
| Option A                |
| **Option B** _(active)_ |
| Option C                |
@vercel
Copy link

vercel bot commented Mar 29, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

headlessui-vue – ./packages/playground-vue

🔍 Inspect: https://vercel.com/tailwindlabs/headlessui-vue/HSaDAzuFg1NdKnXWT254WCYc9PVG
✅ Preview: https://headlessui-vue-git-prevent-double-keydowns-tailwindlabs.vercel.app

headlessui-react – ./packages/playground-react

🔍 Inspect: https://vercel.com/tailwindlabs/headlessui-react/HfRFpXFFSYYSzu8TEGoCeFJRhifa
✅ Preview: https://headlessui-react-git-prevent-double-keydowns-tailwindlabs.vercel.app

@calthomson
Copy link

Seems to be working! Awesome, thanks @RobinMalfait for the quick fix! 🙌 🙌 🙌

@pvandamme
Copy link

pvandamme commented Apr 27, 2022

Hi @RobinMalfait, thanks for the fix, but it's seems that it's still not working when static props is set to true on the Combobox.Options component 😕 I see no issue opened related to this problem, I can create one with more details if you need.

@kytosai
Copy link

kytosai commented May 12, 2022

Hi @RobinMalfait, thanks for the fix, but it's seems that it's still not working when static props is set to true on the Combobox.Options component 😕 I see no issue opened related to this problem, I can create one with more details if you need.

I have the same problem, but my English is not good. So I hope you create an article about this issue. Thank you !

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

Successfully merging this pull request may close these issues.

None yet

4 participants