fix(kselect, kmultiselect): filtered arrow navigation [KHCP-13956] #2573
Conversation
portikM
requested review from
adamdehaven,
jillztom,
Justineo and
a team
as code owners
✅ Deploy Preview for kongponents-sandbox ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kongponents ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| const currentElementIndex = Array.from(selectableItemsElements).findIndex(el => el === target) | ||
| // move to the next or previous element | ||
| const nextElementIndex = key === 'ArrowDown' ? currentElementIndex + 1 : currentElementIndex - 1 | ||
| const nextElement = selectableItemsElements[nextElementIndex] as HTMLButtonElement |
There was a problem hiding this comment.
| const nextElement = selectableItemsElements[nextElementIndex] as HTMLButtonElement | |
| const nextElement = selectableItemsElements[nextElementIndex] |
There was a problem hiding this comment.
nit: Can we use less verbose variable names?
There was a problem hiding this comment.
Now that all logic is contained within a single component, I simplified the variable names
| @@ -469,7 +453,7 @@ const emit = defineEmits<{ | |||
| (e: 'item-removed', value: MultiselectItem): void | |||
| }>() | |||
|
|
|||
| const kMultiselectItemsContainer = ref<HTMLDivElement | null>(null) | |||
| const kMultiselectItems = ref<InstanceType<typeof KMultiselectItems> | null>(null) | |||
There was a problem hiding this comment.
We can switch to useTemplateRef so that this component type can be automatically inferred from the template.
| const itemsContainer = ref<HTMLDivElement | null>(null) | ||
|
|
||
| const setItemFocus = (): void => { | ||
| const firstSelectableItem = itemsContainer.value?.querySelectorAll<HTMLButtonElement>('.multiselect-item button:not([disabled])')[0] |
There was a problem hiding this comment.
Since we are querying the first result here we can use this:
| const firstSelectableItem = itemsContainer.value?.querySelectorAll<HTMLButtonElement>('.multiselect-item button:not([disabled])')[0] | |
| const firstSelectableItem = itemsContainer.value?.querySelector<HTMLButtonElement>('.multiselect-item button:not(:disabled)') |
nit: I would prefer pseudo classes over attribute selectors as they are more semantically precise ([disabled] → :disabled)
| :item="item" | ||
| @keydown="onKeyDown" | ||
| @keydown="onKeyPress" |
There was a problem hiding this comment.
q: why are we renaming this? onKeyDown seems to be more precise.
There was a problem hiding this comment.
At one point I realized I got confused as to why this method name suggests that it only handles down key press (since we use that method only for arrow keys events, it could be mistaken for down arrow key press exclusive handler). onKeyPress IMO makes it clear that the method handles any key press.
| :key="item.key" | ||
| :item="item" | ||
| @keydown="onKeyDown" | ||
| @keydown="onKeyPress" |
There was a problem hiding this comment.
See explanation in my other comment.
portikM
deleted the
fix/khcp-13956-kselect-kmultiselect-filtered-arrow-navigation
branch
## [9.18.2](v9.18.1...v9.18.2) (2025-01-23) ### Bug Fixes * **kselect, kmultiselect:** filtered arrow navigation [KHCP-13956] ([#2573](#2573)) ([7ff96d2](7ff96d2))
|
🎉 This PR is included in version 9.18.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Preview package from this PR in consuming applicationIn consuming application project install preview version of kongponents generated by this PR: |
Summary
Addresses: https://konghq.atlassian.net/browse/KHCP-13956
Fixes broken arrow navigation in KSelect and KMultiselect
Verification steps: