feat(Table): support dynamic sticky styling#12348
Open
kmcfaul wants to merge 3 commits intopatternfly:mainfrom
Open
feat(Table): support dynamic sticky styling#12348kmcfaul wants to merge 3 commits intopatternfly:mainfrom
kmcfaul wants to merge 3 commits intopatternfly:mainfrom
Conversation
WalkthroughThe PR adds ref forwarding to Changes
Sequence DiagramsequenceDiagram
participant ScrollParent as Scroll Parent Container
participant UseHook as useIsStuckFromScrollParent Hook
participant TableComponent as TableStickyHeaderDynamic Component
participant Table as Table Component
participant DOM as DOM (styled output)
Note over ScrollParent,DOM: Initialization & Scroll Event Handling
TableComponent->>UseHook: Provide scrollParent ref & shouldTrack=true
UseHook->>ScrollParent: Read initial scrollTop value
UseHook->>UseHook: Initialize isStuck state (scrollTop > 0)
UseHook->>ScrollParent: Attach passive scroll event listener
ScrollParent->>UseHook: Dispatch scroll event (scrollTop updated)
UseHook->>UseHook: Update isStuck state based on scrollTop
UseHook->>TableComponent: Return updated isStuck value
TableComponent->>Table: Pass isStickyHeaderBase=true & isStickyHeaderStuck=isStuck
Table->>DOM: Conditionally apply stickyHeaderBase & stickyHeaderStuck modifiers
DOM-->>ScrollParent: Render with updated sticky styling
Note over UseHook,DOM: Cleanup on unmount
UseHook->>ScrollParent: Remove scroll event listener
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
Collaborator
|
Preview: https://pf-react-pr-12348.surge.sh A11y report: https://pf-react-pr-12348-a11y.surge.sh |
kmcfaul
force-pushed
the
sticky-table-updates
branch
from
0668f05 to
4316b40
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Contributor
Author
|
Reopening to rerun Snyk |
thatblindgeye
approved these changes
Apr 13, 2026
Contributor
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Nothing blocking for me below
Comment on lines
+423
to
+425
| A sticky header may alternatively be implemented with two properties: `isStickyHeaderBase` and `isStickyHeaderStuck` - which allows separate control of the sticky position and sticky styling. `isStickyHeaderBase` should always be applied to make the header position sticky, and `isStickyHeaderStuck` may be applied dynamically to enable the sticky styling, such as when the sticky header is not at the top of the scroll parent as shown in the example. | ||
|
|
||
| `isStickyHeader` acts as if both properties are present and true when applied, and is useful when dynamic sticky styling is not necessary. |
kmcfaul
force-pushed
the
sticky-table-updates
branch
from
f5177ac to
133e4ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-table/src/components/Table/examples/TableStickyHeaderDynamic.tsx (1)
28-46: UseuseEffectinstead ofuseLayoutEffectfor scroll-listener wiring.This effect subscribes to scroll events and updates state—it doesn't perform DOM measurements or mutations.
useEffectis the appropriate choice and avoids potential SSR-related warnings.Proposed diff
-import { useLayoutEffect, useRef, useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; ... - useLayoutEffect(() => { + useEffect(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-table/src/components/Table/examples/TableStickyHeaderDynamic.tsx` around lines 28 - 46, The effect that wires the scroll listener uses useLayoutEffect but only updates state (setIsStuck) and does not perform DOM measurements; replace useLayoutEffect with useEffect in the component so the scroll listener (syncFromScroll) attached to scrollParentRef.current runs in useEffect, keeping the same cleanup and dependency array ([shouldTrack, scrollParentRef]) and preserving logic around shouldTrack, scrollElement and setIsStuck.
🤖 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 17-25: The parameter type for scrollParentRef in
useIsStuckFromScrollParent is currently React.RefObject<any>, which loses DOM
type safety; change it to React.RefObject<HTMLDivElement> (or an appropriate
HTMLElement subtype) in the function signature and any related usages so callers
and TypeScript know the ref points to a div and you can safely call DOM methods
like addEventListener and scrollTop on scrollParentRef.current.
---
Nitpick comments:
In
`@packages/react-table/src/components/Table/examples/TableStickyHeaderDynamic.tsx`:
- Around line 28-46: The effect that wires the scroll listener uses
useLayoutEffect but only updates state (setIsStuck) and does not perform DOM
measurements; replace useLayoutEffect with useEffect in the component so the
scroll listener (syncFromScroll) attached to scrollParentRef.current runs in
useEffect, keeping the same cleanup and dependency array ([shouldTrack,
scrollParentRef]) and preserving logic around shouldTrack, scrollElement and
setIsStuck.
🪄 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: 0f2cb079-959b-49c1-a113-58254b3eb6c9
📒 Files selected for processing (5)
packages/react-table/src/components/Table/InnerScrollContainer.tsxpackages/react-table/src/components/Table/Table.tsxpackages/react-table/src/components/Table/__tests__/Table.test.tsxpackages/react-table/src/components/Table/examples/Table.mdpackages/react-table/src/components/Table/examples/TableStickyHeaderDynamic.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/react-table/src/components/Table/tests/Table.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react-table/src/components/Table/examples/Table.md
- packages/react-table/src/components/Table/Table.tsx
- packages/react-table/src/components/Table/InnerScrollContainer.tsx
Comment on lines
+17
to
+25
| const useIsStuckFromScrollParent = ({ | ||
| shouldTrack, | ||
| scrollParentRef | ||
| }: { | ||
| /** Indicates whether to track the scroll top position of the scroll parent element */ | ||
| shouldTrack: boolean; | ||
| /** Reference to the scroll parent element */ | ||
| scrollParentRef: React.RefObject<any>; | ||
| }): boolean => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the ref type exposed/expected by InnerScrollContainer and align this example type.
fd -i "InnerScrollContainer.tsx"
rg -n -C3 "forwardRef|InnerScrollContainer|RefObject|HTMLDivElement|HTMLElement" packages/react-table/src/components/Table/InnerScrollContainer.tsx
rg -n -C2 "useIsStuckFromScrollParent|scrollParentRef" packages/react-table/src/components/Table/examples/TableStickyHeaderDynamic.tsxRepository: patternfly/patternfly-react
Length of output: 2319
Replace any on scrollParentRef with concrete DOM type for type safety.
Line 24 uses React.RefObject<any>, losing compile-time checks for DOM APIs like addEventListener. The ref is only used with HTMLDivElement instances.
Proposed diff
- scrollParentRef: React.RefObject<any>;
+ scrollParentRef: React.RefObject<HTMLDivElement>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/react-table/src/components/Table/examples/TableStickyHeaderDynamic.tsx`
around lines 17 - 25, The parameter type for scrollParentRef in
useIsStuckFromScrollParent is currently React.RefObject<any>, which loses DOM
type safety; change it to React.RefObject<HTMLDivElement> (or an appropriate
HTMLElement subtype) in the function signature and any related usages so callers
and TypeScript know the ref points to a div and you can safely call DOM methods
like addEventListener and scrollTop on scrollParentRef.current.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
New Features
Documentation