Open
Conversation
WalkthroughToolbar adds two new beta props, Changes
Sequence DiagramsequenceDiagram
participant User as User
participant ScrollContainer as ScrollContainer
participant Hook as useIsStuckFromScrollParent
participant Toolbar as Toolbar
User->>ScrollContainer: scroll
ScrollContainer->>Hook: passive scroll event
Hook->>Hook: compute isStuck (scrollTop > 0)
Hook->>Toolbar: update prop isStickyStuck
Toolbar->>Toolbar: apply/remove stickyStuck modifier
Toolbar->>User: render updated toolbar
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
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.
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
2 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
Tests
Documentation