feat(Toolbar): dynamic sticky#12375
Merged
thatblindgeye merged 4 commits intopatternfly:mainfrom Apr 28, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtends the Toolbar component with two new beta properties— Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-core/src/components/Toolbar/examples/Toolbar.md`:
- Around line 47-53: Update the Dynamic sticky toolbar docs to note
scroll-parent guidance: explain that isStickyBase controls the CSS sticky
positioning but isStickyStuck must be driven by scroll state from the same
scrollable ancestor that creates the sticky context, and add a short sentence in
the Toolbar.md example (and mention ToolbarDynamicSticky.tsx) instructing
consumers to attach their scroll listener or intersection observer to the
scrollable container (not window) so isStickyStuck reflects that container's
scroll position.
In
`@packages/react-core/src/components/Toolbar/examples/ToolbarDynamicSticky.tsx`:
- Around line 43-44: The demo array currently produces 0-29 so the UI shows
"item 0"; change the array generation used by the constants array and numbers so
it yields 1-30 instead of 0-29 (adjust the mapping in the Array.from call used
to create array), keeping the existing showEvenOnly filter logic (numbers =
showEvenOnly ? array.filter(number => number % 2 === 0) : array) unchanged so
even-only behavior still works with the new 1-30 range.
- Line 47: The scroll container div using scrollParentRef is not
keyboard-focusable or labeled; update the element referenced by scrollParentRef
to be reachable and announced by assistive tech by adding a focusable attribute
(e.g., tabIndex={0}) and an accessible name (e.g., role="region" with aria-label
or aria-labelledby) so keyboard users can focus and scroll the example content;
ensure these attributes are applied to the same div that has style={{ overflowY:
'scroll', height: '200px' }} and preserve existing refs and styles.
🪄 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: b742df8a-4376-410d-9592-1f4f41533973
📒 Files selected for processing (4)
packages/react-core/src/components/Toolbar/Toolbar.tsxpackages/react-core/src/components/Toolbar/__tests__/Toolbar.test.tsxpackages/react-core/src/components/Toolbar/examples/Toolbar.mdpackages/react-core/src/components/Toolbar/examples/ToolbarDynamicSticky.tsx
| const numbers = showEvenOnly ? array.filter((number) => number % 2 === 0) : array; | ||
|
|
||
| return ( | ||
| <div ref={scrollParentRef} style={{ overflowY: 'scroll', height: '200px' }}> |
There was a problem hiding this comment.
Make the scroll container keyboard accessible.
This fixed-height overflow container is the primary interaction target for demonstrating sticky behavior, but it is not focusable or named. Keyboard users may not be able to scroll the example content.
♿ Proposed fix
- <div ref={scrollParentRef} style={{ overflowY: 'scroll', height: '200px' }}>
+ <div
+ ref={scrollParentRef}
+ role="region"
+ aria-label="Scrollable dynamic sticky toolbar example"
+ tabIndex={0}
+ style={{ overflowY: 'scroll', height: '200px' }}
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div ref={scrollParentRef} style={{ overflowY: 'scroll', height: '200px' }}> | |
| <div | |
| ref={scrollParentRef} | |
| role="region" | |
| aria-label="Scrollable dynamic sticky toolbar example" | |
| tabIndex={0} | |
| style={{ overflowY: 'scroll', height: '200px' }} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Toolbar/examples/ToolbarDynamicSticky.tsx`
at line 47, The scroll container div using scrollParentRef is not
keyboard-focusable or labeled; update the element referenced by scrollParentRef
to be reachable and announced by assistive tech by adding a focusable attribute
(e.g., tabIndex={0}) and an accessible name (e.g., role="region" with aria-label
or aria-labelledby) so keyboard users can focus and scroll the example content;
ensure these attributes are applied to the same div that has style={{ overflowY:
'scroll', height: '200px' }} and preserve existing refs and styles.
Collaborator
|
Preview: https://pf-react-pr-12375.surge.sh A11y report: https://pf-react-pr-12375-a11y.surge.sh |
kmcfaul
force-pushed
the
12332-toolbar-dynamic-sticky
branch
from
08eb1ce to
b38a419
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-core/src/components/Toolbar/examples/ToolbarDynamicSticky.tsx (1)
11-11: Tighten thescrollParentReftype.
React.RefObject<any>forgoes type safety. Since the consumer passes anHTMLDivElementref and the hook only readsscrollTop/addEventListener, typing it asHTMLElement(or making the hook generic) would better document intent and catch misuse.♻️ Proposed refactor
- scrollParentRef: React.RefObject<any>; + scrollParentRef: React.RefObject<HTMLElement | null>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Toolbar/examples/ToolbarDynamicSticky.tsx` at line 11, The scrollParentRef is currently typed as React.RefObject<any>; tighten it to a more specific DOM type to improve safety — change scrollParentRef: React.RefObject<any> to React.RefObject<HTMLElement> (or React.RefObject<HTMLDivElement> if the consumer always passes a div), since the code only reads scrollTop and uses addEventListener; alternatively make the hook generic (e.g., <T extends HTMLElement>) and use React.RefObject<T> so callers can supply the precise element type.
🤖 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-core/src/components/Toolbar/examples/ToolbarDynamicSticky.tsx`:
- Around line 42-44: The SearchInput state (searchValue / setSearchValue) is not
used to filter the rendered list: numbers is computed only from array and
showEvenOnly so typing does nothing; fix by applying searchValue when computing
numbers (e.g., update the numbers definition in this file to include a filter
like .filter(n => n.toString().includes(searchValue)) after the showEvenOnly
filter) or remove the SearchInput state and control if you prefer; ensure you
update the symbols searchValue, setSearchValue and numbers (and the initial
array) so the rendered list reflects the search input.
---
Nitpick comments:
In
`@packages/react-core/src/components/Toolbar/examples/ToolbarDynamicSticky.tsx`:
- Line 11: The scrollParentRef is currently typed as React.RefObject<any>;
tighten it to a more specific DOM type to improve safety — change
scrollParentRef: React.RefObject<any> to React.RefObject<HTMLElement> (or
React.RefObject<HTMLDivElement> if the consumer always passes a div), since the
code only reads scrollTop and uses addEventListener; alternatively make the hook
generic (e.g., <T extends HTMLElement>) and use React.RefObject<T> so callers
can supply the precise element type.
🪄 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: 7887a63e-110c-415d-88c5-d2c3da0c31d8
📒 Files selected for processing (2)
packages/react-core/src/components/Toolbar/examples/Toolbar.mdpackages/react-core/src/components/Toolbar/examples/ToolbarDynamicSticky.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/react-core/src/components/Toolbar/examples/Toolbar.md
Comment on lines
+42
to
+44
| const [searchValue, setSearchValue] = useState(''); | ||
| const array = Array.from(Array(30), (_, x) => x); // create array of numbers from 1-30 for demo purposes | ||
| const numbers = showEvenOnly ? array.filter((number) => number % 2 === 0) : array; |
There was a problem hiding this comment.
searchValue has no effect on the rendered list.
The SearchInput is wired to searchValue/setSearchValue, but numbers is filtered only by showEvenOnly — typing in the search input does nothing visible. For a docs example this is likely to confuse readers about how the toolbar controls relate to the content. Either drop the SearchInput state plumbing or apply it to the rendered list.
🐛 Proposed fix
- const numbers = showEvenOnly ? array.filter((number) => number % 2 === 0) : array;
+ const filteredByEven = showEvenOnly ? array.filter((number) => number % 2 === 0) : array;
+ const numbers = searchValue
+ ? filteredByEven.filter((number) => `item ${number}`.includes(searchValue))
+ : filteredByEven;Also applies to: 51-56, 69-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Toolbar/examples/ToolbarDynamicSticky.tsx`
around lines 42 - 44, The SearchInput state (searchValue / setSearchValue) is
not used to filter the rendered list: numbers is computed only from array and
showEvenOnly so typing does nothing; fix by applying searchValue when computing
numbers (e.g., update the numbers definition in this file to include a filter
like .filter(n => n.toString().includes(searchValue)) after the showEvenOnly
filter) or remove the SearchInput state and control if you prefer; ensure you
update the symbols searchValue, setSearchValue and numbers (and the initial
array) so the rendered list reflects the search input.
57df9be to
7df7cb2
Compare
thatblindgeye
approved these changes
Apr 28, 2026
kmcfaul
force-pushed
the
12332-toolbar-dynamic-sticky
branch
from
7df7cb2 to
514cf53
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-core/src/components/Toolbar/Toolbar.tsx (1)
179-181: Define precedence when legacy and beta sticky props are combined.If consumers pass
isSticky,isStickyBase, andisStickyStucktogether, all three modifier classes are applied. Consider enforcing precedence (or documenting it explicitly) to avoid ambiguous styling combinations.♻️ Suggested precedence guard
<div + // Legacy `isSticky` takes precedence over beta split modifiers. + // This prevents mixed sticky class combinations. className={css( styles.toolbar, hasNoPadding && styles.modifiers.noPadding, isFullHeight && styles.modifiers.fullHeight, isStatic && styles.modifiers.static, isSticky && styles.modifiers.sticky, - isStickyBase && styles.modifiers.stickyBase, - isStickyStuck && styles.modifiers.stickyStuck, + !isSticky && isStickyBase && styles.modifiers.stickyBase, + !isSticky && isStickyStuck && styles.modifiers.stickyStuck, isVertical && styles.modifiers.vertical, formatBreakpointMods(inset, styles, '', getBreakpoint(width)), colorVariant === 'primary' && styles.modifiers.primary,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Toolbar/Toolbar.tsx` around lines 179 - 181, Toolbar currently applies all three sticky modifier classes when isSticky, isStickyBase, and isStickyStuck are passed; change the class selection to enforce a clear precedence (e.g., isStickyStuck > isStickyBase > isSticky) so only one modifier from styles.modifiers.{sticky, stickyBase, stickyStuck} is applied. Locate the className/composition logic in the Toolbar component (variables isSticky, isStickyBase, isStickyStuck) and replace the boolean OR application with a precedence check (if isStickyStuck use styles.modifiers.stickyStuck else if isStickyBase use styles.modifiers.stickyBase else if isSticky use styles.modifiers.sticky). Update any relevant tests or documentation strings to reflect the enforced precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-core/src/components/Toolbar/Toolbar.tsx`:
- Around line 179-181: Toolbar currently applies all three sticky modifier
classes when isSticky, isStickyBase, and isStickyStuck are passed; change the
class selection to enforce a clear precedence (e.g., isStickyStuck >
isStickyBase > isSticky) so only one modifier from styles.modifiers.{sticky,
stickyBase, stickyStuck} is applied. Locate the className/composition logic in
the Toolbar component (variables isSticky, isStickyBase, isStickyStuck) and
replace the boolean OR application with a precedence check (if isStickyStuck use
styles.modifiers.stickyStuck else if isStickyBase use
styles.modifiers.stickyBase else if isSticky use styles.modifiers.sticky).
Update any relevant tests or documentation strings to reflect the enforced
precedence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 629299f0-d07e-4af9-8bb8-8806a0793bf0
📒 Files selected for processing (4)
packages/react-core/src/components/Toolbar/Toolbar.tsxpackages/react-core/src/components/Toolbar/__tests__/Toolbar.test.tsxpackages/react-core/src/components/Toolbar/examples/Toolbar.mdpackages/react-core/src/components/Toolbar/examples/ToolbarDynamicSticky.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/react-core/src/components/Toolbar/examples/Toolbar.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-core/src/components/Toolbar/tests/Toolbar.test.tsx
- packages/react-core/src/components/Toolbar/examples/ToolbarDynamicSticky.tsx
mcoker
approved these changes
Apr 28, 2026
Contributor
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Just one small nit, otherwise LGTM
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 #12332
isStickyBaseandisStickyStuckisStickyStuckWaiting on patternfly/patternfly#8321Summary by CodeRabbit
New Features
isStickyBaseandisStickyStuckbeta properties for finer control over sticky toolbar positioning and visual feedback when the toolbar is "stuck" during scrolling.Documentation
Tests