From e2294f5280038d792c8358a0a81f2ef3bb04ece8 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Thu, 26 Jan 2023 15:55:55 +0100 Subject: [PATCH] Improve `Tabs` wrapping around when controlling the component and overflowing the `selectedIndex` (#2213) * ensure chaning the `selectedIndex` tabs properly wraps around We never want to use and index that doesn't map to a proper tab. This commit also makes the implementation similar for both React and Vue. * add tests to prove the underflow and overflow wrapping * drop updating the index manually This is already adjusted when tabs change internally. You can still manually change it of course, but for these tests that doesn't matter and cause different results. * update changelog --- packages/@headlessui-react/CHANGELOG.md | 1 + .../src/components/tabs/tabs.test.tsx | 89 +++++++++++- .../src/components/tabs/tabs.tsx | 39 +++++- packages/@headlessui-vue/CHANGELOG.md | 1 + .../src/components/tabs/tabs.test.ts | 101 +++++++++++++- .../src/components/tabs/tabs.ts | 128 ++++++++++++------ 6 files changed, 312 insertions(+), 47 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index b77c788..a255a34 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Improve `Combobox` accessibility ([#2153](https://github.com/tailwindlabs/headlessui/pull/2153)) - Fix crash when reading `headlessuiFocusGuard` of `relatedTarget` in the `FocusTrap` component ([#2203](https://github.com/tailwindlabs/headlessui/pull/2203)) - Fix `FocusTrap` in `Dialog` when there is only 1 focusable element ([#2172](https://github.com/tailwindlabs/headlessui/pull/2172)) +- Improve `Tabs` wrapping around when controlling the component and overflowing the `selectedIndex` ([#2213](https://github.com/tailwindlabs/headlessui/pull/2213)) ## [1.7.7] - 2022-12-16 diff --git a/packages/@headlessui-react/src/components/tabs/tabs.test.tsx b/packages/@headlessui-react/src/components/tabs/tabs.test.tsx index eb8217e..310f847 100644 --- a/packages/@headlessui-react/src/components/tabs/tabs.test.tsx +++ b/packages/@headlessui-react/src/components/tabs/tabs.test.tsx @@ -179,7 +179,6 @@ describe('Rendering', () => { + + )} + + ) + } + render() + + assertActiveElement(document.body) + + await click(getByText('Next')) + assertTabs({ active: 1 }) + + await click(getByText('Next')) + assertTabs({ active: 2 }) + + await click(getByText('Next')) + assertTabs({ active: 0 }) + + await click(getByText('Next')) + assertTabs({ active: 1 }) + }) + ) + + it( + 'should wrap around when underflowing the index when using a controlled component', + suppressConsoleLogs(async () => { + function Example() { + let [selectedIndex, setSelectedIndex] = useState(0) + + return ( + + {({ selectedIndex }) => ( + <> + + Tab 1 + Tab 2 + Tab 3 + + + Content 1 + Content 2 + Content 3 + + + + )} + + ) + } + render() + + assertActiveElement(document.body) + + await click(getByText('Previous')) + assertTabs({ active: 2 }) + + await click(getByText('Previous')) + assertTabs({ active: 1 }) + + await click(getByText('Previous')) + assertTabs({ active: 0 }) + + await click(getByText('Previous')) + assertTabs({ active: 2 }) + }) + ) }) describe(`'Tab'`, () => { diff --git a/packages/@headlessui-react/src/components/tabs/tabs.tsx b/packages/@headlessui-react/src/components/tabs/tabs.tsx index 4d384fc..1a27038 100644 --- a/packages/@headlessui-react/src/components/tabs/tabs.tsx +++ b/packages/@headlessui-react/src/components/tabs/tabs.tsx @@ -30,6 +30,17 @@ import { microTask } from '../../utils/micro-task' import { Hidden } from '../../internal/hidden' import { getOwnerDocument } from '../../utils/owner' +enum Direction { + Forwards, + Backwards, +} + +enum Ordering { + Less = -1, + Equal = 0, + Greater = 1, +} + interface StateDefinition { selectedIndex: number @@ -68,16 +79,30 @@ let reducers: { let nextState = { ...state, tabs, panels } - // Underflow - if (action.index < 0) { - return { ...nextState, selectedIndex: tabs.indexOf(focusableTabs[0]) } - } + if ( + // Underflow + action.index < 0 || + // Overflow + action.index > tabs.length - 1 + ) { + let direction = match(Math.sign(action.index - state.selectedIndex), { + [Ordering.Less]: () => Direction.Backwards, + [Ordering.Equal]: () => { + return match(Math.sign(action.index), { + [Ordering.Less]: () => Direction.Forwards, + [Ordering.Equal]: () => Direction.Forwards, + [Ordering.Greater]: () => Direction.Backwards, + }) + }, + [Ordering.Greater]: () => Direction.Forwards, + }) - // Overflow - else if (action.index > tabs.length) { return { ...nextState, - selectedIndex: tabs.indexOf(focusableTabs[focusableTabs.length - 1]), + selectedIndex: match(direction, { + [Direction.Forwards]: () => tabs.indexOf(focusableTabs[0]), + [Direction.Backwards]: () => tabs.indexOf(focusableTabs[focusableTabs.length - 1]), + }), } } diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md index cf7fb2a..b31a076 100644 --- a/packages/@headlessui-vue/CHANGELOG.md +++ b/packages/@headlessui-vue/CHANGELOG.md @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Improve `Combobox` accessibility ([#2153](https://github.com/tailwindlabs/headlessui/pull/2153)) - Fix crash when reading `headlessuiFocusGuard` of `relatedTarget` in the `FocusTrap` component ([#2203](https://github.com/tailwindlabs/headlessui/pull/2203)) - Fix `FocusTrap` in `Dialog` when there is only 1 focusable element ([#2172](https://github.com/tailwindlabs/headlessui/pull/2172)) +- Improve `Tabs` wrapping around when controlling the component and overflowing the `selectedIndex` ([#2213](https://github.com/tailwindlabs/headlessui/pull/2213)) ## [1.7.7] - 2022-12-16 diff --git a/packages/@headlessui-vue/src/components/tabs/tabs.test.ts b/packages/@headlessui-vue/src/components/tabs/tabs.test.ts index 588b37f..64d7104 100644 --- a/packages/@headlessui-vue/src/components/tabs/tabs.test.ts +++ b/packages/@headlessui-vue/src/components/tabs/tabs.test.ts @@ -199,7 +199,6 @@ describe('Rendering', () => { selectedIndex, reverse() { tabs.value = tabs.value.slice().reverse() - selectedIndex.value = tabs.value.length - 1 - selectedIndex.value }, handleChange(value: number) { selectedIndex.value = value @@ -999,6 +998,106 @@ describe('`selectedIndex`', () => { assertTabs({ active: 0 }) assertActiveElement(getByText('Tab 1')) }) + + it( + 'should wrap around when overflowing the index when using a controlled component', + suppressConsoleLogs(async () => { + renderTemplate({ + template: html` + + + Tab 1 + Tab 2 + Tab 3 + + + + Content 1 + Content 2 + Content 3 + + + + + `, + setup() { + let value = ref(0) + return { + value, + set(v: number) { + value.value = v + }, + } + }, + }) + + await new Promise(nextTick) + + assertActiveElement(document.body) + + await click(getByText('Next')) + assertTabs({ active: 1 }) + + await click(getByText('Next')) + assertTabs({ active: 2 }) + + await click(getByText('Next')) + assertTabs({ active: 0 }) + + await click(getByText('Next')) + assertTabs({ active: 1 }) + }) + ) + + it( + 'should wrap around when underflowing the index when using a controlled component', + suppressConsoleLogs(async () => { + renderTemplate({ + template: html` + + + Tab 1 + Tab 2 + Tab 3 + + + + Content 1 + Content 2 + Content 3 + + + + + `, + setup() { + let value = ref(0) + return { + value, + set(v: number) { + value.value = v + }, + } + }, + }) + + await new Promise(nextTick) + + assertActiveElement(document.body) + + await click(getByText('Previous')) + assertTabs({ active: 2 }) + + await click(getByText('Previous')) + assertTabs({ active: 1 }) + + await click(getByText('Previous')) + assertTabs({ active: 0 }) + + await click(getByText('Previous')) + assertTabs({ active: 2 }) + }) + ) }) describe('Keyboard interactions', () => { diff --git a/packages/@headlessui-vue/src/components/tabs/tabs.ts b/packages/@headlessui-vue/src/components/tabs/tabs.ts index d903939..507a965 100644 --- a/packages/@headlessui-vue/src/components/tabs/tabs.ts +++ b/packages/@headlessui-vue/src/components/tabs/tabs.ts @@ -13,6 +13,7 @@ import { // Types InjectionKey, Ref, + watch, } from 'vue' import { Features, render, omit } from '../../utils/render' @@ -27,6 +28,17 @@ import { microTask } from '../../utils/micro-task' import { Hidden } from '../../internal/hidden' import { getOwnerDocument } from '../../utils/owner' +enum Direction { + Forwards, + Backwards, +} + +enum Ordering { + Less = -1, + Equal = 0, + Greater = 1, +} + type StateDefinition = { // State selectedIndex: Ref @@ -78,7 +90,9 @@ export let TabGroup = defineComponent({ }, inheritAttrs: false, setup(props, { slots, attrs, emit }) { - let selectedIndex = ref(null) + let selectedIndex = ref( + props.selectedIndex ?? props.defaultIndex + ) let tabs = ref([]) let panels = ref([]) @@ -87,6 +101,60 @@ export let TabGroup = defineComponent({ isControlled.value ? props.selectedIndex : selectedIndex.value ) + function setSelectedIndex(indexToSet: number) { + let tabs = sortByDomNode(api.tabs.value, dom) + let panels = sortByDomNode(api.panels.value, dom) + + let focusableTabs = tabs.filter((tab) => !dom(tab)?.hasAttribute('disabled')) + + if ( + // Underflow + indexToSet < 0 || + // Overflow + indexToSet > tabs.length - 1 + ) { + let direction = match( + selectedIndex.value === null // Not set yet + ? Ordering.Equal + : Math.sign(indexToSet - selectedIndex.value!), + { + [Ordering.Less]: () => Direction.Backwards, + [Ordering.Equal]: () => { + return match(Math.sign(indexToSet), { + [Ordering.Less]: () => Direction.Forwards, + [Ordering.Equal]: () => Direction.Forwards, + [Ordering.Greater]: () => Direction.Backwards, + }) + }, + [Ordering.Greater]: () => Direction.Forwards, + } + ) + + selectedIndex.value = match(direction, { + [Direction.Forwards]: () => tabs.indexOf(focusableTabs[0]), + [Direction.Backwards]: () => tabs.indexOf(focusableTabs[focusableTabs.length - 1]), + }) + api.tabs.value = tabs + api.panels.value = panels + } + + // Middle + else { + let before = tabs.slice(0, indexToSet) + let after = tabs.slice(indexToSet) + + let next = [...after, ...before].find((tab) => focusableTabs.includes(tab)) + if (!next) return + + let localSelectedIndex = tabs.indexOf(next) ?? api.selectedIndex.value + if (localSelectedIndex === -1) localSelectedIndex = api.selectedIndex.value + + selectedIndex.value = localSelectedIndex + api.tabs.value = tabs + api.panels.value = panels + } + } + let api = { selectedIndex: computed(() => selectedIndex.value ?? props.defaultIndex ?? null), orientation: computed(() => (props.vertical ? 'vertical' : 'horizontal')), @@ -99,18 +167,29 @@ export let TabGroup = defineComponent({ } if (!isControlled.value) { - selectedIndex.value = index + setSelectedIndex(index) } }, registerTab(tab: typeof tabs['value'][number]) { - if (!tabs.value.includes(tab)) tabs.value.push(tab) + if (tabs.value.includes(tab)) return + let activeTab = tabs.value[selectedIndex.value!] + + tabs.value.push(tab) + tabs.value = sortByDomNode(tabs.value, dom) + + let localSelectedIndex = tabs.value.indexOf(activeTab) ?? selectedIndex.value + if (localSelectedIndex !== -1) { + selectedIndex.value = localSelectedIndex + } }, unregisterTab(tab: typeof tabs['value'][number]) { let idx = tabs.value.indexOf(tab) if (idx !== -1) tabs.value.splice(idx, 1) }, registerPanel(panel: typeof panels['value'][number]) { - if (!panels.value.includes(panel)) panels.value.push(panel) + if (panels.value.includes(panel)) return + panels.value.push(panel) + panels.value = sortByDomNode(panels.value, dom) }, unregisterPanel(panel: typeof panels['value'][number]) { let idx = panels.value.indexOf(panel) @@ -130,41 +209,14 @@ export let TabGroup = defineComponent({ computed(() => (mounted.value ? null : SSRCounter.value)) ) - watchEffect(() => { - if (api.tabs.value.length <= 0) return - if (props.selectedIndex === null && selectedIndex.value !== null) return + let incomingSelectedIndex = computed(() => props.selectedIndex) - api.tabs.value = sortByDomNode(api.tabs.value, dom) - api.panels.value = sortByDomNode(api.panels.value, dom) - - let tabs = api.tabs.value.map((tab) => dom(tab)).filter(Boolean) as HTMLElement[] - let focusableTabs = tabs.filter((tab) => !tab.hasAttribute('disabled')) - - let indexToSet = props.selectedIndex ?? props.defaultIndex - - // Underflow - if (indexToSet < 0) { - selectedIndex.value = tabs.indexOf(focusableTabs[0]) - } - - // Overflow - else if (indexToSet > api.tabs.value.length) { - selectedIndex.value = tabs.indexOf(focusableTabs[focusableTabs.length - 1]) - } - - // Middle - else { - let before = tabs.slice(0, indexToSet) - let after = tabs.slice(indexToSet) - - let next = [...after, ...before].find((tab) => focusableTabs.includes(tab)) - if (!next) return - - let localSelectedIndex = tabs.indexOf(next) ?? api.selectedIndex.value - if (localSelectedIndex === -1) localSelectedIndex = api.selectedIndex.value - - selectedIndex.value = localSelectedIndex - } + onMounted(() => { + watch( + [incomingSelectedIndex /* Deliberately skipping defaultIndex */], + () => setSelectedIndex(props.selectedIndex ?? props.defaultIndex), + { immediate: true } + ) }) watchEffect(() => {