fix(kselect, kmultiselect): filtered arrow navigation [KHCP-13956] #2573

portikM · 2025-01-15T17:54:04Z

Summary

Addresses: https://konghq.atlassian.net/browse/KHCP-13956

Fixes broken arrow navigation in KSelect and KMultiselect

Verification steps:

Find a sortable instance of KSelect and/or KMultiselect in sandbox or docs
Type a query in and after the component shows the results, remove it
Try navigating through items using up and down arrows - it should work correctly

netlify · 2025-01-15T17:54:29Z

✅ Deploy Preview for kongponents-sandbox ready!

Name	Link
🔨 Latest commit	`79745bf`
🔍 Latest deploy log	https://app.netlify.com/sites/kongponents-sandbox/deploys/67898e3e39e03000083fdc66
😎 Deploy Preview	https://deploy-preview-2573--kongponents-sandbox.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2025-01-15T17:56:37Z

✅ Deploy Preview for kongponents ready!

Name	Link
🔨 Latest commit	`79745bf`
🔍 Latest deploy log	https://app.netlify.com/sites/kongponents/deploys/67898e3e59b6240008c3342a
😎 Deploy Preview	https://deploy-preview-2573--kongponents.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Justineo · 2025-01-16T10:07:48Z

src/components/KMultiselect/KMultiselectItems.vue

@@ -72,7 +72,7 @@ const getGroupItems = (group: string) => props.items?.filter(item => item.group
 const setFocus = (index: number = 0) => {
  if (kMultiselectItem.value) {
    if (!props.items[index].disabled) {
-      kMultiselectItem.value[index]?.$el?.querySelector('button').focus()
+      kMultiselectItem.value[index]?.$el?.querySelector('button')?.focus()


I believe we should move away from using the refs array. A straightforward alternative would be to use querySelectorAll to select the .select-item button elements, as this approach always reflects the correct order. This change also eliminates the need to add a key to the K(Multi)SelectItems component, which can cause unnecessary re-rendering on every item change.

I've refactored the implementation moving away from using the refs array to utilize querySelectorAll like you suggested. Please review and let me know what you think

kongponents-bot · 2025-01-16T22:57:21Z

Preview package from this PR in consuming application

In consuming application project install preview version of kongponents generated by this PR:

@kong/kongponents@pr-2573

Justineo

I left some comments on KMultiselect, which I believe also apply to KSelect.

Justineo · 2025-01-17T16:20:42Z

src/components/KMultiselect/KMultiselect.vue

+              <div
+                ref="kMultiselectItemsContainer"
+                aria-live="polite"
+                class="multiselect-items-container"


How about moving this element into <KMultiselectItems>?

Justineo · 2025-01-17T16:26:02Z

src/components/KMultiselect/KMultiselectItems.vue

+const onKeyDown = (event: Event) => {
+  const { target, key } = event as KeyboardEvent


Suggested change

const onKeyDown = (event: Event) => {

const { target, key } = event as KeyboardEvent

const onKeyDown = ({ target, key } : KeyboardEvent) => {

Justineo · 2025-01-17T16:43:57Z

src/components/KMultiselect/KMultiselectItems.vue

-  const index = props.items.findIndex(item => item.key === key)
+  if (key === 'ArrowDown' || key === 'ArrowUp') {
+    // find the items container element
+    const kSelectItemsContainer = (target as HTMLElement).closest('.multiselect-items-container')


I think it might be better to move the wrapper into this component so that we don't query for elements outside this component.

Justineo · 2025-01-17T16:51:16Z

src/components/KMultiselect/KMultiselect.vue

+  }
+}
+
+const setArrowNavigationFocus = (): void => {


I think we should still provide a focus method on <KMultiselectItems> so that we don't need to query into another component.

Justineo · 2025-01-17T16:54:58Z

src/components/KMultiselect/KMultiselectItems.vue

-  }
+    if (selectableItemsElements?.length) {
+      // find the current element index in the array
+      const currentElementIndex = Array.from(selectableItemsElements).findIndex(el => el === target)


Suggested change

const currentElementIndex = Array.from(selectableItemsElements).findIndex(el => el === target)

const currentElementIndex = [...selectableItemsElements].indexOf(target)

Justineo · 2025-01-17T16:56:10Z

src/components/KMultiselect/KMultiselectItems.vue

+    // find the items container element
+    const kSelectItemsContainer = (target as HTMLElement).closest('.multiselect-items-container')
+    // all selectable items
+    const selectableItemsElements = kSelectItemsContainer?.querySelectorAll('.multiselect-item button:not([disabled])')


Suggested change

const selectableItemsElements = kSelectItemsContainer?.querySelectorAll('.multiselect-item button:not([disabled])')

const selectableItemsElements = kSelectItemsContainer?.querySelectorAll<HTMLButtonElement>('.multiselect-item button:not(:disabled)')

Justineo · 2025-01-17T16:57:53Z

src/components/KMultiselect/KMultiselectItems.vue

+      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


Suggested change

const nextElement = selectableItemsElements[nextElementIndex] as HTMLButtonElement

const nextElement = selectableItemsElements[nextElementIndex]

nit: Can we use less verbose variable names?

Justineo · 2025-01-17T16:58:16Z

src/components/KMultiselect/KMultiselectItems.vue

+      if (nextElement) {
+        nextElement.focus()
+      }


Suggested change

if (nextElement) {

nextElement.focus()

}

nextElement?.focus()

fix(kselect, kmultiselect): filtered arrow navigation [KHCP-13956]

b8ca16a

portikM self-assigned this Jan 15, 2025

portikM requested review from adamdehaven, jillztom, Justineo and a team as code owners January 15, 2025 17:54

Justineo reviewed Jan 16, 2025

View reviewed changes

portikM added 4 commits January 16, 2025 17:29

fix(kselect): refactor arrow key navigation implementation [KHCP-13956]

adca9ce

fix(kselect): minor fix [KHCP-13956]

549d4de

fix(kmultiselect): refactor arrow key nav implementation [KHCP-13956]

ee64279

fix: minor fix

79745bf

Justineo reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(kselect, kmultiselect): filtered arrow navigation [KHCP-13956] #2573

fix(kselect, kmultiselect): filtered arrow navigation [KHCP-13956] #2573

portikM commented Jan 15, 2025

netlify bot commented Jan 15, 2025 •

edited

Loading

netlify bot commented Jan 15, 2025 •

edited

Loading

Justineo Jan 16, 2025

portikM Jan 16, 2025

kongponents-bot commented Jan 16, 2025

Justineo left a comment

Justineo Jan 17, 2025

Justineo Jan 17, 2025

Justineo Jan 17, 2025

Justineo Jan 17, 2025

Justineo Jan 17, 2025

Justineo Jan 17, 2025

Justineo Jan 17, 2025

Justineo Jan 17, 2025

Justineo Jan 17, 2025

		const onKeyDown = (event: Event) => {
		const { target, key } = event as KeyboardEvent

	const onKeyDown = (event: Event) => {
	const { target, key } = event as KeyboardEvent
	const onKeyDown = ({ target, key } : KeyboardEvent) => {

	const currentElementIndex = Array.from(selectableItemsElements).findIndex(el => el === target)
	const currentElementIndex = [...selectableItemsElements].indexOf(target)

	const selectableItemsElements = kSelectItemsContainer?.querySelectorAll('.multiselect-item button:not([disabled])')
	const selectableItemsElements = kSelectItemsContainer?.querySelectorAll<HTMLButtonElement>('.multiselect-item button:not(:disabled)')

	const nextElement = selectableItemsElements[nextElementIndex] as HTMLButtonElement
	const nextElement = selectableItemsElements[nextElementIndex]

fix(kselect, kmultiselect): filtered arrow navigation [KHCP-13956] #2573

Are you sure you want to change the base?

fix(kselect, kmultiselect): filtered arrow navigation [KHCP-13956] #2573

Conversation

portikM commented Jan 15, 2025

Summary

netlify bot commented Jan 15, 2025 • edited Loading

✅ Deploy Preview for kongponents-sandbox ready!

netlify bot commented Jan 15, 2025 • edited Loading

✅ Deploy Preview for kongponents ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kongponents-bot commented Jan 16, 2025

Preview package from this PR in consuming application

Justineo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Jan 15, 2025 •

edited

Loading

netlify bot commented Jan 15, 2025 •

edited

Loading