feat(Table): add composable sticky footer - #12645
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughAdds sticky footer support to ChangesSticky footer support
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change adds opt-in sticky table footers with a semantic footer component, documentation, and examples. Existing behavior remains unchanged by default, and no current merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant TableStickyFooter
participant InnerScrollContainer
participant Table
participant Tfoot
TableStickyFooter->>InnerScrollContainer: Render fixed-height scroll container
TableStickyFooter->>Table: Enable isStickyFooter
TableStickyFooter->>Tfoot: Render total row
Table->>Table: Apply stickyFooter modifier
Tfoot->>Tfoot: Render styled tfoot element
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rebeccaalpert
left a comment
rebeccaalpert
left a comment
There was a problem hiding this comment.
Just a couple of very very small nitpicky things! Otherwise it looks good to me. :)
| return ( | ||
| <div style={{ height: '400px' }}> | ||
| <InnerScrollContainer> | ||
| <Table aria-label="Sticky footer table" gridBreakPoint="" isStickyFooter> |
There was a problem hiding this comment.
| <Table aria-label="Sticky footer table" gridBreakPoint="" isStickyFooter> | |
| <Table aria-label="Sticky footer table" isStickyFooter> |
| </table> | ||
| ); | ||
|
|
||
| expect(screen.getByText('Footer').closest('tfoot')).toHaveClass(styles.tableTfoot); |
There was a problem hiding this comment.
| expect(screen.getByText('Footer').closest('tfoot')).toHaveClass(styles.tableTfoot); | |
| expect(screen.getByText('Footer').closest('tfoot')).toHaveClass(styles.tableTfoot, { exact: true }); |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
In addition to the file comments below, just a couple more tests to add in:
- One checking that Tfoot renders without children (The
Accordion.test.tsxfile's first test can be used as reference) - Another test checking that children are rendered properly (can just do something like
<Tfoot>Footer content</Tfoot>rather than needing to use semantic table elements like tr or td.
| test('Renders a tfoot element with the table footer class', () => { | ||
| render( | ||
| <table> | ||
| <Tfoot> | ||
| <tr> | ||
| <td>Footer</td> | ||
| </tr> | ||
| </Tfoot> | ||
| </table> | ||
| ); | ||
|
|
||
| expect(screen.getByText('Footer').closest('tfoot')).toHaveClass(styles.tableTfoot); |
There was a problem hiding this comment.
In addition to Rebeccas's comment above:
| test('Renders a tfoot element with the table footer class', () => { | |
| render( | |
| <table> | |
| <Tfoot> | |
| <tr> | |
| <td>Footer</td> | |
| </tr> | |
| </Tfoot> | |
| </table> | |
| ); | |
| expect(screen.getByText('Footer').closest('tfoot')).toHaveClass(styles.tableTfoot); | |
| test(`Renders with class ${styles.tableTfoot} only by default`, () => { | |
| render( | |
| <table> | |
| <Tfoot /> | |
| </table> | |
| ); | |
| expect(screen.getByRole('rowgroup')).toHaveClass(styles.tableTfoot, { exact: true }); | |
| }); |
We want to try and use matchers that can more precisely get the element we want. Since we don't need to worry about any other rowgroup (thead or tbody) we should be okay to use that matcher here. Also just a minor tweak to the test name to match similar tests in other test files.
| test('Forwards props, class names, and refs to the tfoot element', () => { | ||
| const ref = { current: null } as React.RefObject<HTMLTableSectionElement>; | ||
|
|
||
| render( | ||
| <table> | ||
| <Tfoot ref={ref} className="custom-footer" data-testid="footer"> | ||
| <tr /> | ||
| </Tfoot> | ||
| </table> | ||
| ); | ||
|
|
||
| expect(screen.getByTestId('footer')).toHaveClass(styles.tableTfoot, 'custom-footer'); | ||
| expect(ref.current).toBe(screen.getByTestId('footer')); | ||
| }); |
There was a problem hiding this comment.
Can we separate these out into 3 different tests? One for the ref, one for the custom class, and one for the props spreading? Also just using the getByRole matcher from above if we can.
3 participants
What: Adds support for composable sticky table footers.
Jira: PF-3996
Tfootcomponent for semantic table footer rows.isStickyFooterprop toTable.Tfootfrom@patternfly/react-table.Tfootand sticky footer behavior.Validation:
yarn jest packages/react-table/src/components/Table/__tests__/Table.test.tsx packages/react-table/src/components/Table/__tests__/Tfoot.test.tsx --runInBandyarn tsc --build packages/tsconfig.json --pretty falseBoth checks passed.
Additional issues: None.
Summary by CodeRabbit
New Features
isStickyFooterproperty.Documentation