feat(ToggleGroup): added full width variant#12374
Merged
kmcfaul merged 7 commits intopatternfly:mainfrom Apr 24, 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:
WalkthroughThis pull request adds a new Changes
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 |
Collaborator
|
Preview: https://pf-react-pr-12374.surge.sh A11y report: https://pf-react-pr-12374-a11y.surge.sh |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/react-core/src/components/Compass/__tests__/CompassPanel.test.tsx (1)
5-86:⚠️ Potential issue | 🟠 MajorMass
test.skipmasks regressions rather than documenting intent.Every test in this suite is now skipped, which hides the behavioral regression introduced in
CompassPanel.tsx(commented-out modifier classes) from CI. A couple of concerns:
- Skipped tests accumulate and rarely get re-enabled. If the styling removal is intentional, delete the obsolete assertions; if it's temporary, leave a TODO with an issue link on each
test.skipexplaining why.- The
Renders with additional props spread to the componentandRenders with childrentests do not depend on the removed CSS modifiers and should still pass — no reason to skip them.- This change is also outside the stated PR scope (ToggleGroup
isFill).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Compass/__tests__/CompassPanel.test.tsx` around lines 5 - 86, The test suite for CompassPanel improperly marks all tests as skipped, hiding regressions; restore relevant tests and either remove or document obsolete assertions: re-enable tests that do not depend on removed CSS modifiers (specifically the "Renders with children" and "Renders with additional props spread to the component" tests) by removing test.skip on those cases, and for tests that assert modifier classes (tests referencing styles.compassPanel and styles.modifiers.* and the snapshot tests), either delete their assertions if the styling removal is intentional or leave them skipped but replace test.skip with a commented TODO linking an issue and explaining why (or update expectations to match the new CompassPanel behavior in CompassPanel.tsx); ensure you reference the CompassPanel component and the exact test names above when making changes so CI reflects intended behavior.packages/react-core/src/components/Compass/CompassPanel.tsx (1)
1-49:⚠️ Potential issue | 🔴 CriticalOut-of-scope change that silently breaks the CompassPanel public API.
This PR's stated objective is adding an
isFillvariant toToggleGroup(issue#12373), but this file disables the entire styling behavior ofCompassPanel:
- The
@patternfly/react-stylescompass import and all modifier classes (compassPanel,pill,noBorder,noPadding,fullHeight,scrollable) are commented out rather than removed.isPill,hasNoBorder,hasNoPadding,isFullHeight, andisScrollableremain documented inCompassPanelPropsand are destructured from props as comments — so consumers can still pass them, but they will be silently ignored and also fall through to the DOM via...props(causing React unknown-DOM-attribute warnings for the boolean props).- Downstream usage such as
packages/react-core/src/demos/Compass/examples/CompassDemo.tsx(<CompassPanel isPill hasNoPadding>) andCompassMainHeader(which forwardscompassPanelProps) will render without the expected styling.If these changes are intentional, they should be a separate PR that (a) removes the unused props from
CompassPanelPropsand the commented-out lines rather than leaving dead code, and (b) updates consumers/snapshots accordingly. Otherwise, please revert this file from theisFillPR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Compass/CompassPanel.tsx` around lines 1 - 49, The CompassPanel file accidentally disabled its styling and left boolean props to leak into the DOM: restore the original behavior by re-enabling the import from '@patternfly/react-styles/css/components/Compass/compass' (or the appropriate styles object) and reapply the classnames using styles.compassPanel and the modifier classes (e.g., styles.modifiers.pill, noBorder, noPadding, fullHeight, scrollable) inside the css(...) call in CompassPanel; ensure CompassPanelProps still declares isPill, hasNoBorder, hasNoPadding, isFullHeight, isScrollable and destructure them from the component signature (remove them from the spread ...props so they are not passed as unknown DOM attributes), or if the change was intentional, remove those props from CompassPanelProps and callers instead and delete the commented lines—update CompassPanel (and consuming usage like CompassDemo/CompassMainHeader) accordingly.packages/react-core/src/components/Compass/__tests__/CompassMainHeader.test.tsx (1)
77-89:⚠️ Potential issue | 🟠 MajorSkipping these tests hides a real regression.
Both skipped cases assert that
CompassPanelrenders with thestyles.compassPanelclass whentitle/toolbarare passed viaCompassMainHeader. SinceCompassPanel.tsxno longer applies that class, the tests would fail — skipping them only conceals the behavior change. Either restore the class inCompassPanelor delete these tests (and update the snapshot on line 125-128, which will also capture the regression). As noted above, this looks out of scope for theToggleGroupisFillPR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Compass/__tests__/CompassMainHeader.test.tsx` around lines 77 - 89, Tests for CompassMainHeader were skipped but they reveal a regression: CompassPanel no longer receives the styles.compassPanel class when title or toolbar is passed, hiding the behavior change; either restore the class in CompassPanel (ensure CompassPanel component applies styles.compassPanel when rendered via CompassMainHeader) or remove these tests and update the related snapshot to reflect the intended output, and re-run tests to confirm CompassMainHeader and CompassPanel behavior (look for CompassMainHeader, CompassPanel, and styles.compassPanel in the code to locate the change).
🤖 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/ToggleGroup/examples/ToggleGroup.md`:
- Line 70: Update the heading text "Full width toggle group" to hyphenate the
compound adjective so it reads "Full-width toggle group" wherever that exact
heading appears (e.g., in the ToggleGroup examples markdown heading). Ensure the
updated heading string is used in the ToggleGroup.md example section to reflect
proper compound-adjective styling.
In `@packages/react-core/src/components/ToggleGroup/examples/ToggleGroupFill.tsx`:
- Around line 13-24: The three handlers handleItemClickBasic,
handleItemClickMulti, and handleItemClickDisabled have untyped event parameters;
annotate each event parameter as the union type expected by ToggleGroupItem:
React.MouseEvent<any> | React.KeyboardEvent | MouseEvent (and keep the existing
isSelected: boolean param on handleItemClickMulti), so change the signatures for
handleItemClickBasic(event), handleItemClickMulti(event, isSelected: boolean),
and handleItemClickDisabled(event) to use that union type to satisfy
noImplicitAny; you don't need a default React import—just add the type
annotations to the function parameters.
---
Outside diff comments:
In
`@packages/react-core/src/components/Compass/__tests__/CompassMainHeader.test.tsx`:
- Around line 77-89: Tests for CompassMainHeader were skipped but they reveal a
regression: CompassPanel no longer receives the styles.compassPanel class when
title or toolbar is passed, hiding the behavior change; either restore the class
in CompassPanel (ensure CompassPanel component applies styles.compassPanel when
rendered via CompassMainHeader) or remove these tests and update the related
snapshot to reflect the intended output, and re-run tests to confirm
CompassMainHeader and CompassPanel behavior (look for CompassMainHeader,
CompassPanel, and styles.compassPanel in the code to locate the change).
In `@packages/react-core/src/components/Compass/__tests__/CompassPanel.test.tsx`:
- Around line 5-86: The test suite for CompassPanel improperly marks all tests
as skipped, hiding regressions; restore relevant tests and either remove or
document obsolete assertions: re-enable tests that do not depend on removed CSS
modifiers (specifically the "Renders with children" and "Renders with additional
props spread to the component" tests) by removing test.skip on those cases, and
for tests that assert modifier classes (tests referencing styles.compassPanel
and styles.modifiers.* and the snapshot tests), either delete their assertions
if the styling removal is intentional or leave them skipped but replace
test.skip with a commented TODO linking an issue and explaining why (or update
expectations to match the new CompassPanel behavior in CompassPanel.tsx); ensure
you reference the CompassPanel component and the exact test names above when
making changes so CI reflects intended behavior.
In `@packages/react-core/src/components/Compass/CompassPanel.tsx`:
- Around line 1-49: The CompassPanel file accidentally disabled its styling and
left boolean props to leak into the DOM: restore the original behavior by
re-enabling the import from
'@patternfly/react-styles/css/components/Compass/compass' (or the appropriate
styles object) and reapply the classnames using styles.compassPanel and the
modifier classes (e.g., styles.modifiers.pill, noBorder, noPadding, fullHeight,
scrollable) inside the css(...) call in CompassPanel; ensure CompassPanelProps
still declares isPill, hasNoBorder, hasNoPadding, isFullHeight, isScrollable and
destructure them from the component signature (remove them from the spread
...props so they are not passed as unknown DOM attributes), or if the change was
intentional, remove those props from CompassPanelProps and callers instead and
delete the commented lines—update CompassPanel (and consuming usage like
CompassDemo/CompassMainHeader) accordingly.
🪄 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: d0752e2f-1d10-49fe-a871-9d9abbc9964a
⛔ Files ignored due to path filters (3)
packages/react-core/src/components/Compass/__tests__/__snapshots__/CompassMainHeader.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Compass/__tests__/__snapshots__/CompassPanel.test.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
packages/react-core/package.jsonpackages/react-core/src/components/Compass/CompassPanel.tsxpackages/react-core/src/components/Compass/__tests__/CompassMainHeader.test.tsxpackages/react-core/src/components/Compass/__tests__/CompassPanel.test.tsxpackages/react-core/src/components/ToggleGroup/ToggleGroup.tsxpackages/react-core/src/components/ToggleGroup/__tests__/ToggleGroup.test.tsxpackages/react-core/src/components/ToggleGroup/examples/ToggleGroup.mdpackages/react-core/src/components/ToggleGroup/examples/ToggleGroupFill.tsxpackages/react-docs/package.jsonpackages/react-icons/package.jsonpackages/react-styles/package.jsonpackages/react-tokens/package.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ToggleGroup/examples/ToggleGroup.md`:
- Line 72: Update the example description string for the ToggleGroup isFill
example to hyphenate compound adjectives: change phrases like "full width",
"single select", and "multi select" to "full-width", "single-select", and
"multi-select" so the sentence reads e.g. "The following example shows
full-width toggle groups for a single-select, multi-select, and single-select
with disabled item." Target the text around the ToggleGroup isFill example in
ToggleGroup.md.(Position reference: the sentence describing full width toggle
groups.)
🪄 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: 4d62b466-47b9-45fc-ab34-9c3c879992a6
📒 Files selected for processing (1)
packages/react-core/src/components/ToggleGroup/examples/ToggleGroup.md
kmcfaul
approved these changes
Apr 23, 2026
thatblindgeye
force-pushed
the
iss12373_toggleFill
branch
from
28819c8 to
7826f0c
Compare
Collaborator
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
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
5 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 #12373
Additional issues:
Summary by CodeRabbit
New Features
isFillprop to ToggleGroup component for full-width display.Documentation
Tests
Chores