From 4cef591e332799d8257e86106881e471951642b8 Mon Sep 17 00:00:00 2001 From: GitHub Date: Tue, 17 Dec 2024 21:15:49 +1100 Subject: [PATCH 1/5] Fix tabs auto selection --- .../tabs/src/useTabListState.ts | 4 +- .../react-aria-components/test/Tabs.test.js | 94 ++++++++++++++++++- 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/packages/@react-stately/tabs/src/useTabListState.ts b/packages/@react-stately/tabs/src/useTabListState.ts index 74a55aaf09e..5c0d52ab419 100644 --- a/packages/@react-stately/tabs/src/useTabListState.ts +++ b/packages/@react-stately/tabs/src/useTabListState.ts @@ -43,7 +43,7 @@ export function useTabListState(props: TabListStateOptions) useEffect(() => { // Ensure a tab is always selected (in case no selected key was specified or if selected item was deleted from collection) let selectedKey = currentSelectedKey; - if (selectionManager.isEmpty || selectedKey == null || !collection.getItem(selectedKey)) { + if ((selectionManager.isEmpty || selectedKey == null || !collection.getItem(selectedKey))) { selectedKey = findDefaultSelectedKey(collection, state.disabledKeys); if (selectedKey != null) { // directly set selection because replace/toggle selection won't consider disabled keys @@ -56,7 +56,7 @@ export function useTabListState(props: TabListStateOptions) selectionManager.setFocusedKey(selectedKey); } lastSelectedKey.current = selectedKey; - }); + }, [collection]); return { ...state, diff --git a/packages/react-aria-components/test/Tabs.test.js b/packages/react-aria-components/test/Tabs.test.js index 5b471156605..3782fc73c9e 100644 --- a/packages/react-aria-components/test/Tabs.test.js +++ b/packages/react-aria-components/test/Tabs.test.js @@ -11,8 +11,8 @@ */ import {act, fireEvent, pointerMap, render, waitFor, within} from '@react-spectrum/test-utils-internal'; -import React from 'react'; -import {Tab, TabList, TabPanel, Tabs} from '../'; +import React, {useState} from 'react'; +import {Button, Collection, Tab, TabList, TabPanel, Tabs} from '../'; import {TabsExample} from '../stories/Tabs.stories'; import userEvent from '@testing-library/user-event'; @@ -474,4 +474,94 @@ describe('Tabs', () => { expect(innerTabs[0]).toHaveTextContent('One'); expect(innerTabs[1]).toHaveTextContent('Two'); }); + + it('can add tabs and keep the current selected key', async () => { + let onSelectionChange = jest.fn(); + function Example(props) { + let [tabs, setTabs] = useState([ + { id: 1, title: "Tab 1", content: "Tab body 1" }, + { id: 2, title: "Tab 2", content: "Tab body 2" }, + { id: 3, title: "Tab 3", content: "Tab body 3" }, + ]); + + const [selectedTabId, setSelectedTabId] = useState(tabs[0].id); + + let addTab = () => { + const tabId = tabs.length + 1; + + setTabs((prevTabs) => [ + ...prevTabs, + { + id: tabId, + title: `Tab ${tabId}`, + content: `Tab body ${tabId}`, + }, + ]); + + // Use functional update to ensure you're working with the most recent state + setSelectedTabId((prevSelectedTabId) => tabId); + }; + + let removeTab = () => { + if (tabs.length > 1) { + setTabs((prevTabs) => { + const updatedTabs = prevTabs.slice(0, -1); + // Update selectedTabId to the last remaining tab's ID if the current selected tab is removed + const newSelectedTabId = updatedTabs[updatedTabs.length - 1].id; + setSelectedTabId(newSelectedTabId); + return updatedTabs; + }); + } + }; + + const onSelectionChange = (value) => { + setSelectedTabId(value); + props.onSelectionChange(value); + }; + + return ( + +
+ + {(item) => ( + + {({ isSelected }) => ( +

+ {item.title} +

+ )} + + )} + +
+ + +
+
+ + {(item) => ( + + {item.content} + + )} + + + ); + } + render(); + await user.tab(); + await user.keyboard('{ArrowRight}'); + await user.tab(); + onSelectionChange.mockClear(); + await user.keyboard('{Enter}'); + expect(onSelectionChange).not.toHaveBeenCalled(); + }); }); From b83901a9f2ccc8b181fc8fad830ce9d0ca6f5048 Mon Sep 17 00:00:00 2001 From: GitHub Date: Tue, 17 Dec 2024 21:21:34 +1100 Subject: [PATCH 2/5] fix lint --- packages/@react-spectrum/utils/src/Slots.tsx | 2 +- .../tabs/src/useTabListState.ts | 2 +- .../react-aria-components/test/Tabs.test.js | 28 +++++++++---------- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/packages/@react-spectrum/utils/src/Slots.tsx b/packages/@react-spectrum/utils/src/Slots.tsx index e910ccd7048..d6aa6204ccf 100644 --- a/packages/@react-spectrum/utils/src/Slots.tsx +++ b/packages/@react-spectrum/utils/src/Slots.tsx @@ -36,7 +36,7 @@ export function cssModuleToSlots(cssModule) { export function SlotProvider(props) { const emptyObj = useMemo(() => ({}), []); - // eslint-disable-next-line react-hooks/exhaustive-deps + let parentSlots = useContext(SlotContext) || emptyObj; let {slots = emptyObj, children} = props; diff --git a/packages/@react-stately/tabs/src/useTabListState.ts b/packages/@react-stately/tabs/src/useTabListState.ts index 5c0d52ab419..16406f8c100 100644 --- a/packages/@react-stately/tabs/src/useTabListState.ts +++ b/packages/@react-stately/tabs/src/useTabListState.ts @@ -43,7 +43,7 @@ export function useTabListState(props: TabListStateOptions) useEffect(() => { // Ensure a tab is always selected (in case no selected key was specified or if selected item was deleted from collection) let selectedKey = currentSelectedKey; - if ((selectionManager.isEmpty || selectedKey == null || !collection.getItem(selectedKey))) { + if (selectionManager.isEmpty || selectedKey == null || !collection.getItem(selectedKey)) { selectedKey = findDefaultSelectedKey(collection, state.disabledKeys); if (selectedKey != null) { // directly set selection because replace/toggle selection won't consider disabled keys diff --git a/packages/react-aria-components/test/Tabs.test.js b/packages/react-aria-components/test/Tabs.test.js index 3782fc73c9e..3fb79f21212 100644 --- a/packages/react-aria-components/test/Tabs.test.js +++ b/packages/react-aria-components/test/Tabs.test.js @@ -11,8 +11,8 @@ */ import {act, fireEvent, pointerMap, render, waitFor, within} from '@react-spectrum/test-utils-internal'; -import React, {useState} from 'react'; import {Button, Collection, Tab, TabList, TabPanel, Tabs} from '../'; +import React, {useState} from 'react'; import {TabsExample} from '../stories/Tabs.stories'; import userEvent from '@testing-library/user-event'; @@ -479,9 +479,9 @@ describe('Tabs', () => { let onSelectionChange = jest.fn(); function Example(props) { let [tabs, setTabs] = useState([ - { id: 1, title: "Tab 1", content: "Tab body 1" }, - { id: 2, title: "Tab 2", content: "Tab body 2" }, - { id: 3, title: "Tab 3", content: "Tab body 3" }, + {id: 1, title: 'Tab 1', content: 'Tab body 1'}, + {id: 2, title: 'Tab 2', content: 'Tab body 2'}, + {id: 3, title: 'Tab 3', content: 'Tab body 3'} ]); const [selectedTabId, setSelectedTabId] = useState(tabs[0].id); @@ -494,8 +494,8 @@ describe('Tabs', () => { { id: tabId, title: `Tab ${tabId}`, - content: `Tab body ${tabId}`, - }, + content: `Tab body ${tabId}` + } ]); // Use functional update to ensure you're working with the most recent state @@ -521,16 +521,15 @@ describe('Tabs', () => { return ( -
- +
+ {(item) => ( - {({ isSelected }) => ( + {({isSelected}) => (

+ color: isSelected ? 'red' : 'black' + }}> {item.title}

)} @@ -546,9 +545,8 @@ describe('Tabs', () => { {(item) => ( + borderTop: '2px solid black' + }}> {item.content} )} From 93d67702c594849a5cb228bade9f9243ebf5e476 Mon Sep 17 00:00:00 2001 From: GitHub Date: Wed, 18 Dec 2024 08:58:03 +1100 Subject: [PATCH 3/5] see if it passes on CI --- packages/@react-stately/tabs/src/useTabListState.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/@react-stately/tabs/src/useTabListState.ts b/packages/@react-stately/tabs/src/useTabListState.ts index 16406f8c100..21d3517b92c 100644 --- a/packages/@react-stately/tabs/src/useTabListState.ts +++ b/packages/@react-stately/tabs/src/useTabListState.ts @@ -40,10 +40,11 @@ export function useTabListState(props: TabListStateOptions) } = state; let lastSelectedKey = useRef(currentSelectedKey); + let lastCollection = useRef(collection); useEffect(() => { // Ensure a tab is always selected (in case no selected key was specified or if selected item was deleted from collection) let selectedKey = currentSelectedKey; - if (selectionManager.isEmpty || selectedKey == null || !collection.getItem(selectedKey)) { + if (lastCollection.current !== collection && (selectionManager.isEmpty || selectedKey == null || !collection.getItem(selectedKey))) { selectedKey = findDefaultSelectedKey(collection, state.disabledKeys); if (selectedKey != null) { // directly set selection because replace/toggle selection won't consider disabled keys @@ -56,7 +57,8 @@ export function useTabListState(props: TabListStateOptions) selectionManager.setFocusedKey(selectedKey); } lastSelectedKey.current = selectedKey; - }, [collection]); + lastCollection.current = collection; + }); return { ...state, From 3183d8c5633e6436bc7f6aeb80cda50ccd00d0a7 Mon Sep 17 00:00:00 2001 From: GitHub Date: Wed, 18 Dec 2024 10:00:43 +1100 Subject: [PATCH 4/5] remove forced selection in controlled --- packages/@react-stately/tabs/src/useTabListState.ts | 4 +--- packages/react-aria-components/test/Tabs.test.js | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/@react-stately/tabs/src/useTabListState.ts b/packages/@react-stately/tabs/src/useTabListState.ts index 21d3517b92c..c9bc6c3c23d 100644 --- a/packages/@react-stately/tabs/src/useTabListState.ts +++ b/packages/@react-stately/tabs/src/useTabListState.ts @@ -40,11 +40,10 @@ export function useTabListState(props: TabListStateOptions) } = state; let lastSelectedKey = useRef(currentSelectedKey); - let lastCollection = useRef(collection); useEffect(() => { // Ensure a tab is always selected (in case no selected key was specified or if selected item was deleted from collection) let selectedKey = currentSelectedKey; - if (lastCollection.current !== collection && (selectionManager.isEmpty || selectedKey == null || !collection.getItem(selectedKey))) { + if (props.selectedKey == null && (selectionManager.isEmpty || selectedKey == null || !collection.getItem(selectedKey))) { selectedKey = findDefaultSelectedKey(collection, state.disabledKeys); if (selectedKey != null) { // directly set selection because replace/toggle selection won't consider disabled keys @@ -57,7 +56,6 @@ export function useTabListState(props: TabListStateOptions) selectionManager.setFocusedKey(selectedKey); } lastSelectedKey.current = selectedKey; - lastCollection.current = collection; }); return { diff --git a/packages/react-aria-components/test/Tabs.test.js b/packages/react-aria-components/test/Tabs.test.js index 3fb79f21212..d93c6f52843 100644 --- a/packages/react-aria-components/test/Tabs.test.js +++ b/packages/react-aria-components/test/Tabs.test.js @@ -499,7 +499,7 @@ describe('Tabs', () => { ]); // Use functional update to ensure you're working with the most recent state - setSelectedTabId((prevSelectedTabId) => tabId); + setSelectedTabId(tabId); }; let removeTab = () => { From 3b67f389f2229e460ff86b70d56efe2f09a5b432 Mon Sep 17 00:00:00 2001 From: GitHub Date: Thu, 23 Jan 2025 13:19:35 +1100 Subject: [PATCH 5/5] add a little more to the test --- packages/react-aria-components/test/Tabs.test.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/react-aria-components/test/Tabs.test.js b/packages/react-aria-components/test/Tabs.test.js index 86fa927be62..c617756f258 100644 --- a/packages/react-aria-components/test/Tabs.test.js +++ b/packages/react-aria-components/test/Tabs.test.js @@ -577,12 +577,22 @@ describe('Tabs', () => { ); } - render(); + let {getAllByRole} = render(); + let tabs = getAllByRole('tab'); await user.tab(); await user.keyboard('{ArrowRight}'); + expect(tabs[1]).toHaveAttribute('aria-selected', 'true'); await user.tab(); onSelectionChange.mockClear(); await user.keyboard('{Enter}'); expect(onSelectionChange).not.toHaveBeenCalled(); + tabs = getAllByRole('tab'); + expect(tabs[3]).toHaveAttribute('aria-selected', 'true'); + + await user.tab(); + await user.keyboard('{Enter}'); + expect(onSelectionChange).not.toHaveBeenCalled(); + tabs = getAllByRole('tab'); + expect(tabs[2]).toHaveAttribute('aria-selected', 'true'); }); });