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

Jump to bottom

Update useLayoutEffect dependencies to ensure TabLine resizes #3688

Merged
merged 9 commits into from Jan 19, 2023
Merged

Update useLayoutEffect dependencies to ensure TabLine resizes #3688

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9032a0e
Update useLayoutEffect dependency array to ensure TabLine resizes pro…
tywayne Oct 27, 2022
2cb2ffe
useResizeObserver to catch any changes to selectedTab size
tywayne Oct 27, 2022
b8059f6
Merge branch 'main' into issue-2004
reidbarber Nov 21, 2022
856d73c
Merge branch 'main' into issue-2004
LFDanLu Nov 22, 2022
1b84c88
Merge branch 'main' into issue-2004
snowystinger Jan 18, 2023
70007d6
handle other tabs pushing the selected tab
snowystinger Jan 18, 2023
6b4715a
fix lint
snowystinger Jan 18, 2023
8bb2141
Merge branch 'main' into issue-2004
LFDanLu Jan 19, 2023
aaecc81
Merge branch 'main' into issue-2004
snowystinger Jan 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 12 additions & 2 deletions packages/@react-spectrum/tabs/src/Tabs.tsx
View file
Open in desktop
Expand Up @@ -210,7 +210,12 @@ function TabLine(props: TabLineProps) {
height: undefined
});

useLayoutEffect(() => {
let selectedTabRef = useRef<HTMLElement>();
if (selectedTab) {
selectedTabRef.current = selectedTab;
}

let onResize = useCallback(() => {
if (selectedTab) {
let styleObj = {transform: undefined, width: undefined, height: undefined};
// In RTL, calculate the transform from the right edge of the tablist so that resizing the window doesn't break the Tabline position due to offsetLeft changes
Expand All @@ -226,8 +231,13 @@ function TabLine(props: TabLineProps) {
}
setStyle(styleObj);
}
}, [direction, setStyle, selectedTab, orientation]);

useLayoutEffect(() => {
onResize();
}, [onResize, direction, scale, selectedKey, orientation, selectedTab?.offsetLeft]);
Copy link
Member

Choose a reason for hiding this comment

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

Think we can get rid of some of these extra deps since they are already in the callback's dep array

Suggested change
}, [onResize, direction, scale, selectedKey, orientation, selectedTab?.offsetLeft]);
}, [onResize, scale, selectedKey, selectedTab?.offsetLeft]);

Copy link
Contributor 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 so—from my understanding, the useCallback deps gives us a memoized callback function with non-stale values. And the useLayoutEffect deps ensure we call that resize function any time its deps change.

Copy link
Member

Choose a reason for hiding this comment

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

@LFDanLu is correct, the onResize function will change anytime its dep array changes (as you said, it's memoized based on that dep array), which will mean that it changes in this dependency array and cause the useLayoutEffect to run

however, I have a different concern. we still have offsetLeft in this dependency array, which means we're getting lucky and render happens to be running after some change in the browser rendered dom. we should never have a dom rendered measurement as a direct reference in our dependency arrays, it's only safe to read dom values during effects. Here's an overly simplified version of why, https://codesandbox.io/s/rough-butterfly-98ny0r?file=/src/App.js

Instead, we need to be figuring out what actually changed (a resize event?) and hooking up to that to get offsetLeft into state. This will ensure a re-render and therefore the line will be placed correctly. We may need to move the state up a component in order to accomplish this. I haven't looked into it, just noticed it when I scanned the review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yep! Not sure how I looked right over onResize being in the second dep array, haha.

I think I'm following your other concern. Can take another look sometime soon, though likely not until after holiday.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine, no rush. I've been trying to find time to look into it as well. Thanks for all the work you've done so far


}, [direction, setStyle, selectedTab, orientation, scale, selectedKey]);
useResizeObserver({ref: selectedTabRef, onResize: onResize});

return <div className={classNames(styles, 'spectrum-Tabs-selectionIndicator')} role="presentation" style={style} />;
}
Expand Down
22 changes: 22 additions & 0 deletions packages/@react-spectrum/tabs/stories/Tabs.stories.tsx
View file
Open in desktop
Expand Up @@ -250,6 +250,28 @@ storiesOf('Tabs', module)
);
}
)
.add(
'Tab 1 controlled TabList item',
() => {
let [tab1Text, setTab1Text] = useState('Tab 1');

return (
<Flex minHeight={400} minWidth={400} direction="column">
<TextField label="Tab Title" value={tab1Text} onChange={setTab1Text} />
<Tabs maxWidth={500}>
<TabList>
<Item>{tab1Text}</Item>
<Item>Tab 2</Item>
</TabList>
<TabPanels>
<Item>Tab 1 Content</Item>
<Item>Tab 2 Content</Item>
</TabPanels>
</Tabs>
</Flex>
);
}
)
.add(
'changing selection programatically',
() => (
Expand Down