Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wip: fix: tablist auto selection #7529

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

wip: fix: tablist auto selection #7529

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4cef591
Fix tabs auto selection
snowystinger Dec 17, 2024
b83901a
fix lint
snowystinger Dec 17, 2024
93d6770
see if it passes on CI
snowystinger Dec 17, 2024
3183d8c
remove forced selection in controlled
snowystinger Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@react-spectrum/utils/src/Slots.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion packages/@react-stately/tabs/src/useTabListState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export function useTabListState<T extends object>(props: TabListStateOptions<T>)
selectionManager.setFocusedKey(selectedKey);
}
lastSelectedKey.current = selectedKey;
});
}, [collection]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need this to also update when selectedKey changes, or if selectionManager.isEmpty or a bunch of other conditions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I'd like to get more at the root cause

I'd hoped to change the first if in the effect to include

lastCollection.current !== collection

and then leave off the dependency array, but it's not working for some reason. probably should have opened as a draft, but I forgot

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snowystinger That's weird, that works for me 🤷

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? tests all pass?
have I messed up my own logic?

  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))) {
...
    lastSelectedKey.current = selectedKey;
    lastCollection.current = collection;
  });

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, all tests pass exactly like this 😅 Also did a clean install and cleared the parcel cache just to make sure...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get 8 failures... even on a brand new pull. lets see if CI disagrees with me.

Copy link
Contributor

@nwidynski nwidynski Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I misunderstood what you meant by all tests 😓

Running the whole test pipeline i also get 8 failures in Tabs.test.js. I was running this test file in isolation, so it appears to be some sort of cascading failure because the DOM isn't cleared.

Can you check whether running just Tabs.test.js also passes for you?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a cascade failure, we have more behavior tests in React Spectrum, those are the ones failing, for the reasons outlined by @devongovett

The crux of the issue is that the selectedkey is set at the same time as the item with that key is added.
When the selectedkey changes (regardless of the collection changing), we need to check to see if we should set a forced default since "no tab selected" is not an option.
At this point though, the collection isn't updated, that takes one more render pass.

As a result, the new key doesn't exist when we go to check if we need to set a default, so we set a default which triggers onSelectionChange.
Then the collection is ready, but it's too late.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now, thanks 👍


return {
...state,
Expand Down
92 changes: 90 additions & 2 deletions packages/react-aria-components/test/Tabs.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {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';

Expand Down Expand Up @@ -474,4 +474,92 @@ 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't prevSelectedTabId not being used a lint error?
Does the most recent state matter if we're setting the state to the newly created 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 (
<Tabs selectedKey={selectedTabId} onSelectionChange={onSelectionChange}>
<div style={{display: 'flex'}}>
<TabList aria-label="Dynamic tabs" items={tabs} style={{flex: 1}}>
{(item) => (
<Tab>
{({isSelected}) => (
<p
style={{
color: isSelected ? 'red' : 'black'
}}>
{item.title}
</p>
)}
</Tab>
)}
</TabList>
<div className="button-group">
<Button onPress={addTab}>Add tab</Button>
<Button onPress={removeTab}>Remove tab</Button>
</div>
</div>
<Collection items={tabs}>
{(item) => (
<TabPanel
style={{
borderTop: '2px solid black'
}}>
{item.content}
</TabPanel>
)}
</Collection>
</Tabs>
);
}
render(<Example onSelectionChange={onSelectionChange} />);
await user.tab();
await user.keyboard('{ArrowRight}');
await user.tab();
onSelectionChange.mockClear();
await user.keyboard('{Enter}');
expect(onSelectionChange).not.toHaveBeenCalled();
Copy link
Member

@ktabors ktabors Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we confirm which tab is selected?

});
});
Loading