From 6598db59b7c2d78d00bab4c9c9314a526ddb79e6 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 26 Jul 2022 17:34:14 +0200 Subject: [PATCH] Ensure `by` comparison is used in `multiple` mode (#1717) * use the `compare` function in multiple mode * add tests to verify fix of incorrect `by` behaviour * improve TypeScript types for the `by` prop * update changelog --- packages/@headlessui-react/CHANGELOG.md | 2 +- .../src/components/combobox/combobox.test.tsx | 78 +++++++++++++++++++ .../src/components/combobox/combobox.tsx | 14 ++-- .../src/components/listbox/listbox.test.tsx | 78 +++++++++++++++++++ .../src/components/listbox/listbox.tsx | 11 ++- packages/@headlessui-vue/CHANGELOG.md | 2 +- .../src/components/combobox/combobox.test.ts | 78 +++++++++++++++++++ .../src/components/combobox/combobox.ts | 4 +- .../src/components/listbox/listbox.test.tsx | 78 +++++++++++++++++++ 9 files changed, 330 insertions(+), 15 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index f9c82edb81..6c668cbbb3 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `by` prop for `Listbox`, `Combobox` and `RadioGroup` ([#1482](https://github.com/tailwindlabs/headlessui/pull/1482)) +- Add `by` prop for `Listbox`, `Combobox` and `RadioGroup` ([#1482](https://github.com/tailwindlabs/headlessui/pull/1482), [#1717](https://github.com/tailwindlabs/headlessui/pull/1717)) - Add `@headlessui/tailwindcss` plugin ([#1487](https://github.com/tailwindlabs/headlessui/pull/1487)) ### Fixed diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx index 163d7e2906..4dd5dd594c 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx @@ -272,6 +272,84 @@ describe('Rendering', () => { ) }) ) + + it( + 'should be possible to use completely new objects while rendering (single mode)', + suppressConsoleLogs(async () => { + function Example() { + let [value, setValue] = useState({ id: 2, name: 'Bob' }) + + return ( + + Trigger + + alice + bob + charlie + + + ) + } + + render() + + await click(getComboboxButton()) + let [alice, bob, charlie] = getComboboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + + await click(getComboboxOptions()[2]) + await click(getComboboxButton()) + ;[alice, bob, charlie] = getComboboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'true') + + await click(getComboboxOptions()[1]) + await click(getComboboxButton()) + ;[alice, bob, charlie] = getComboboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + }) + ) + + it( + 'should be possible to use completely new objects while rendering (multiple mode)', + suppressConsoleLogs(async () => { + function Example() { + let [value, setValue] = useState([{ id: 2, name: 'Bob' }]) + + return ( + + Trigger + + alice + bob + charlie + + + ) + } + + render() + + await click(getComboboxButton()) + + await click(getComboboxOptions()[2]) + let [alice, bob, charlie] = getComboboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).toHaveAttribute('aria-selected', 'true') + + await click(getComboboxOptions()[2]) + ;[alice, bob, charlie] = getComboboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + }) + ) }) }) diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx index 41fb2d8290..c7e0c73b22 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.tsx @@ -315,7 +315,7 @@ let ComboboxRoot = forwardRefWithAs(function Combobox< > & { value: TType onChange(value: TType): void - by?: (keyof TType & string) | ((a: TType, z: TType) => boolean) + by?: (keyof TActualType & string) | ((a: TActualType, z: TActualType) => boolean) disabled?: boolean __demoMode?: boolean name?: string @@ -356,19 +356,19 @@ let ComboboxRoot = forwardRefWithAs(function Combobox< let compare = useEvent( typeof by === 'string' - ? (a: TType, z: TType) => { - let property = by as unknown as keyof TType + ? (a: TActualType, z: TActualType) => { + let property = by as unknown as keyof TActualType return a[property] === z[property] } : by ) - let isSelected: (value: TType) => boolean = useCallback( + let isSelected: (value: TActualType) => boolean = useCallback( (compareValue) => match(data.mode, { [ValueMode.Multi]: () => - (value as unknown as TType[]).some((option) => compare(option, compareValue)), - [ValueMode.Single]: () => compare(value, compareValue), + (value as unknown as TActualType[]).some((option) => compare(option, compareValue)), + [ValueMode.Single]: () => compare(value as unknown as TActualType, compareValue), }), [value] ) @@ -500,7 +500,7 @@ let ComboboxRoot = forwardRefWithAs(function Combobox< [ValueMode.Multi]() { let copy = (data.value as TActualType[]).slice() - let idx = copy.indexOf(value as TActualType) + let idx = copy.findIndex((item) => compare(item, value as TActualType)) if (idx === -1) { copy.push(value as TActualType) } else { diff --git a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx index 8fe7eb1f3c..df432e1d6a 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx @@ -263,6 +263,84 @@ describe('Rendering', () => { ) }) ) + + it( + 'should be possible to use completely new objects while rendering (single mode)', + suppressConsoleLogs(async () => { + function Example() { + let [value, setValue] = useState({ id: 2, name: 'Bob' }) + + return ( + + Trigger + + alice + bob + charlie + + + ) + } + + render() + + await click(getListboxButton()) + let [alice, bob, charlie] = getListboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + + await click(getListboxOptions()[2]) + await click(getListboxButton()) + ;[alice, bob, charlie] = getListboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'true') + + await click(getListboxOptions()[1]) + await click(getListboxButton()) + ;[alice, bob, charlie] = getListboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + }) + ) + + it( + 'should be possible to use completely new objects while rendering (multiple mode)', + suppressConsoleLogs(async () => { + function Example() { + let [value, setValue] = useState([{ id: 2, name: 'Bob' }]) + + return ( + + Trigger + + alice + bob + charlie + + + ) + } + + render() + + await click(getListboxButton()) + + await click(getListboxOptions()[2]) + let [alice, bob, charlie] = getListboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).toHaveAttribute('aria-selected', 'true') + + await click(getListboxOptions()[2]) + ;[alice, bob, charlie] = getListboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + }) + ) }) }) diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx index 1c1c6472ff..974cb38e44 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.tsx @@ -315,7 +315,7 @@ let ListboxRoot = forwardRefWithAs(function Listbox< > & { value: TType onChange(value: TType): void - by?: (keyof TType & string) | ((a: TType, z: TType) => boolean) + by?: (keyof TActualType & string) | ((a: TActualType, z: TActualType) => boolean) disabled?: boolean horizontal?: boolean name?: string @@ -345,8 +345,8 @@ let ListboxRoot = forwardRefWithAs(function Listbox< mode: multiple ? ValueMode.Multi : ValueMode.Single, compare: useEvent( typeof by === 'string' - ? (a: TType, z: TType) => { - let property = by as unknown as keyof TType + ? (a: TActualType, z: TActualType) => { + let property = by as unknown as keyof TActualType return a[property] === z[property] } : by @@ -377,7 +377,10 @@ let ListboxRoot = forwardRefWithAs(function Listbox< [ValueMode.Multi]() { let copy = (propsRef.current.value as TActualType[]).slice() - let idx = copy.indexOf(value as TActualType) + let { compare } = propsRef.current + let idx = copy.findIndex((item) => + compare(item as unknown as TActualType, value as TActualType) + ) if (idx === -1) { copy.push(value as TActualType) } else { diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md index 7c50b87d00..cdaaf530a8 100644 --- a/packages/@headlessui-vue/CHANGELOG.md +++ b/packages/@headlessui-vue/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add `by` prop for `Listbox`, `Combobox` and `RadioGroup` ([#1482](https://github.com/tailwindlabs/headlessui/pull/1482)) +- Add `by` prop for `Listbox`, `Combobox` and `RadioGroup` ([#1482](https://github.com/tailwindlabs/headlessui/pull/1482), [#1717](https://github.com/tailwindlabs/headlessui/pull/1717)) - Add `@headlessui/tailwindcss` plugin ([#1487](https://github.com/tailwindlabs/headlessui/pull/1487)) ### Fixed diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts index 0f3512adfd..5454fb89a5 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts @@ -308,6 +308,84 @@ describe('Rendering', () => { ) }) ) + + it( + 'should be possible to use completely new objects while rendering (single mode)', + suppressConsoleLogs(async () => { + renderTemplate({ + template: html` + + Trigger + + alice + bob + charlie + + + `, + setup: () => { + let value = ref({ id: 2, name: 'Bob' }) + return { options, value } + }, + }) + + await click(getComboboxButton()) + let [alice, bob, charlie] = getComboboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + + await click(getComboboxOptions()[2]) + await click(getComboboxButton()) + ;[alice, bob, charlie] = getComboboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'true') + + await click(getComboboxOptions()[1]) + await click(getComboboxButton()) + ;[alice, bob, charlie] = getComboboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + }) + ) + + it( + 'should be possible to use completely new objects while rendering (multiple mode)', + suppressConsoleLogs(async () => { + renderTemplate({ + template: html` + + Trigger + + alice + bob + charlie + + + `, + setup: () => { + let value = ref([{ id: 2, name: 'Bob' }]) + return { options, value } + }, + }) + + await click(getComboboxButton()) + + await click(getComboboxOptions()[2]) + let [alice, bob, charlie] = getComboboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).toHaveAttribute('aria-selected', 'true') + + await click(getComboboxOptions()[2]) + ;[alice, bob, charlie] = getComboboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + }) + ) }) }) diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.ts b/packages/@headlessui-vue/src/components/combobox/combobox.ts index c1ab391d94..09e76832c6 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.ts @@ -316,7 +316,7 @@ export let Combobox = defineComponent({ let copy = toRaw(api.value.value as unknown[]).slice() let raw = toRaw(dataRef.value) - let idx = copy.indexOf(raw) + let idx = copy.findIndex((value) => api.compare(raw, toRaw(value))) if (idx === -1) { copy.push(raw) } else { @@ -341,7 +341,7 @@ export let Combobox = defineComponent({ let copy = toRaw(api.value.value as unknown[]).slice() let raw = toRaw(dataRef.value) - let idx = copy.indexOf(raw) + let idx = copy.findIndex((value) => api.compare(raw, toRaw(value))) if (idx === -1) { copy.push(raw) } else { diff --git a/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx b/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx index 628ea28a5e..71d0ee77eb 100644 --- a/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx +++ b/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx @@ -288,6 +288,84 @@ describe('Rendering', () => { ) }) ) + + it( + 'should be possible to use completely new objects while rendering (single mode)', + suppressConsoleLogs(async () => { + renderTemplate({ + template: html` + + Trigger + + alice + bob + charlie + + + `, + setup: () => { + let value = ref({ id: 2, name: 'Bob' }) + return { options, value } + }, + }) + + await click(getListboxButton()) + let [alice, bob, charlie] = getListboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + + await click(getListboxOptions()[2]) + await click(getListboxButton()) + ;[alice, bob, charlie] = getListboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'true') + + await click(getListboxOptions()[1]) + await click(getListboxButton()) + ;[alice, bob, charlie] = getListboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + }) + ) + + it( + 'should be possible to use completely new objects while rendering (multiple mode)', + suppressConsoleLogs(async () => { + renderTemplate({ + template: html` + + Trigger + + alice + bob + charlie + + + `, + setup: () => { + let value = ref([{ id: 2, name: 'Bob' }]) + return { options, value } + }, + }) + + await click(getListboxButton()) + + await click(getListboxOptions()[2]) + let [alice, bob, charlie] = getListboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).toHaveAttribute('aria-selected', 'true') + + await click(getListboxOptions()[2]) + ;[alice, bob, charlie] = getListboxOptions() + expect(alice).not.toHaveAttribute('aria-selected') + expect(bob).toHaveAttribute('aria-selected', 'true') + expect(charlie).not.toHaveAttribute('aria-selected') + }) + ) }) })