-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Index table sticky header fixes #11617
Conversation
stickyTableHeadings.current.forEach((heading, index) => { | ||
let minWidth = 0; | ||
if (index === 0 && (!isBreakpointsXS() || !selectable)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: Logic elsewhere disables selecting when on mobile breakpoints, so this logic doesn't need replicating again, and hence we can later remove the isBreakpointsXS
function below.
@@ -1221,36 +1184,6 @@ function IndexTableBase({ | |||
handleSelectionChange(SelectionType.Page, checked); | |||
} | |||
|
|||
function renderStickyHeading(heading: IndexTableHeading, index: number) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function has been merged with renderHeading
above since they were meant to be identical but had diverged in ways that introduced bugs in the sticky heading. By combining them, it ensures no divergence and consistent UX.
!selectable && styles['TableHeading-unselectable'], | ||
heading.flush && styles['TableHeading-flush'], | ||
); | ||
function renderHeading( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged renderStickyHeading
function into renderHeading
since they were meant to be identical but had diverged in ways that introduced bugs in the sticky heading. By combining them, it ensures no divergence and consistent UX.
47fa094
to
4a46d98
Compare
04777fa
to
8773a8f
Compare
@@ -729,7 +729,7 @@ describe('<IndexTable>', () => { | |||
</IndexTable>, | |||
); | |||
|
|||
expect(index.findAll(Tooltip)[2].prop('content')).toBe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous sticky header code was rendering a duplicate tooltip when the first column was sticky and sortable, so to select the "second" column header, the test had to pick out the 3rd Tooltip element.
The changes in this PR remove that unnecessary duplicate Tooltip, and so the test selector can be corrected.
Removed myself as a reviewer since i contributed to this PR |
@@ -724,7 +694,7 @@ function IndexTableBase({ | |||
const sharedMarkup = ( | |||
<> | |||
<EventListener event="resize" handler={handleResize} /> | |||
<AfterInitialMount>{stickyHeaderMarkup}</AfterInitialMount> | |||
{stickyHeaderMarkup} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component is just a useEffect
in disguise.
But we rely on a useLayoutEffect
to read the DOM and measure the elements rendered by stickyHeaderMarkup
.
As per React's docs, useLayoutEffect
is run synchronously immediately after DOM tree reconciliation (but before paint) whereas useEffect
is run _a_synchronously at some time in the future.
So the code in useLayoutEffect
tries to read DOM elements which haven't been rendered yet (and wont be until after the useEffect
has run), thereby breaking all the things.
Some git investigation reveals that this useEffect
was inserted to balance out some other code which attempted to set the size of the sticky header elements on every render (so delaying the render was beneficial to allow the regular headings to fully render first).
This PR moves the resizing of sticky header elements to a more appropriate place (inside that useLayoutEffect
), so there's no need to delay the rendering of the sticky header elements, and hence we don't need <AfterInitialMount>
's useEffect
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is some really lovely work here @jesstelford @gwyneplaine, I had noticed that we were having some strange behaviours when using a combination of sticky headings / tooltips / sortable headings, and I cannot seem to replicate those behaviours on the storybook here, which is fantastic!
Would be great to see this as a snapshot in a Spinstance before approving 🚀
/snapit |
🫰✨ Thanks @jesstelford! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/[email protected] yarn add @shopify/[email protected] |
@mrcthms here's a spin instance with the above snapshot installed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tophatted in Spin and Storybook and all looks great y'all 🙌🏽
'@shopify/polaris': patch | ||
--- | ||
|
||
[IndexTable] Unify sticky table header rendering with regular heading for consistency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[IndexTable] Unify sticky table header rendering with regular heading for consistency | |
Unified logic for sticky and fixed column sticky `IndexTable` header row styles |
…hen horizontal scroll
297c770
to
a898303
Compare
/snapit |
🫰✨ Thanks @jesstelford! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/[email protected] yarn add @shopify/[email protected] yarn add @shopify/[email protected] |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/[email protected] ### Minor Changes - [#11802](#11802) [`3d93f8daf`](3d93f8d) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Added `useHover`, `useFocus`, `useFocusIn`, and `useMediaQuery` hooks for building Copy to Clipboard actions - [#11801](#11801) [`f6308995e`](f630899) Thanks [@sophschneider](https://github.com/sophschneider)! - Added white alpha ramp and more dark experimental tokens ### Patch Changes - [#11617](#11617) [`2ff9427b3`](2ff9427) Thanks [@jesstelford](https://github.com/jesstelford)! - [IndexTable] Unify sticky table header rendering with regular heading for consistency - Updated dependencies \[[`f6308995e`](f630899)]: - @shopify/[email protected] ## @shopify/[email protected] ### Minor Changes - [#11801](#11801) [`f6308995e`](f630899) Thanks [@sophschneider](https://github.com/sophschneider)! - Added white alpha ramp and more dark experimental tokens ## @shopify/[email protected] ### Patch Changes - Updated dependencies \[[`f6308995e`](f630899)]: - @shopify/[email protected] - @shopify/[email protected] ## @shopify/[email protected] ### Patch Changes - Updated dependencies \[[`f6308995e`](f630899)]: - @shopify/[email protected] ## [email protected] ### Patch Changes - Updated dependencies \[[`f6308995e`](f630899)]: - @shopify/[email protected] ## [email protected] ### Patch Changes - Updated dependencies \[[`3d93f8daf`](3d93f8d), [`2ff9427b3`](2ff9427), [`f6308995e`](f630899)]: - @shopify/[email protected] - @shopify/[email protected] Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Fixes #11042
Fixes a couple of bugs in the
IndexTable
's vertical sticky header implementation: