feat(Table): support dynamic sticky styling#12348
feat(Table): support dynamic sticky styling#12348kmcfaul wants to merge 3 commits intopatternfly:mainfrom
Conversation
WalkthroughAdds devDependency bumps for Changes
Sequence DiagramsequenceDiagram
participant ScrollParent as Scroll Parent (InnerScrollContainer)
participant Hook as useIsStuckFromScrollParent
participant Component as TableStickyHeaderDynamic
participant Table as Table Component
participant Styles as CSS Modifiers
ScrollParent->>Hook: scroll event (passive)
activate Hook
Hook->>Hook: read scrollTop, compute isStuck
Hook-->>Component: set isStuck state
deactivate Hook
Component->>Table: render with isStickyHeaderBase=true, isStickyHeaderStuck=isStuck
Table->>Styles: apply stickyHeaderBase / stickyHeaderStuck modifiers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Preview: https://pf-react-pr-12348.surge.sh A11y report: https://pf-react-pr-12348-a11y.surge.sh |
0668f05 to
4316b40
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/react-table/src/components/Table/examples/TableStickyHeaderDynamic.tsx`:
- Around line 108-111: In the Tbody of the TableStickyHeaderDynamic component
replace the non-header cell currently rendered with <Th ...> (the state cell
where `{` ${fact.state}`}` is rendered) with a data cell component <Td ...> so
body rows use Td not Th; keep the same props (modifier="nowrap",
dataLabel={columnNames.state}) and children (BlueprintIcon and the state text)
to preserve styling and accessibility semantics.
- Line 84: The aria-label on the example component TableStickyHeaderDynamic.tsx
currently reads "Sticky columns and header table" but the demo only shows
dynamic sticky header behavior; update the aria-label prop (the JSX attribute
aria-label on the TableStickyHeaderDynamic example component) to accurately
describe the example (e.g., something indicating "Dynamic sticky header table"
or similar) so it no longer references sticky columns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1443169-49cc-4b17-a403-8960c219ab96
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
packages/react-core/package.jsonpackages/react-docs/package.jsonpackages/react-icons/package.jsonpackages/react-styles/package.jsonpackages/react-table/src/components/Table/InnerScrollContainer.tsxpackages/react-table/src/components/Table/Table.tsxpackages/react-table/src/components/Table/examples/Table.mdpackages/react-table/src/components/Table/examples/TableStickyHeaderDynamic.tsxpackages/react-tokens/package.json
packages/react-table/src/components/Table/examples/TableStickyHeaderDynamic.tsx
Outdated
Show resolved
Hide resolved
packages/react-table/src/components/Table/examples/TableStickyHeaderDynamic.tsx
Outdated
Show resolved
Hide resolved
|
Reopening to rerun Snyk |
What: Closes #12329
Needs patternfly/patternfly#8223 to be merged and pulled in for styling.refthrough toInnerScrollContainerisStickyHeaderBaseandisStickyHeaderStuckproperties toTableNotes -
New styling is most noticeable in glass theme, but still works in default.
If sticky column support goes in before this merges will update that here.
Summary by CodeRabbit
Chores
New Features
Documentation