feat(Compass): remove background props, update structure#12408
Open
kmcfaul wants to merge 4 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCompass drops ChangesCompass Background Configuration & Header Prop Rename
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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-12408.surge.sh A11y report: https://pf-react-pr-12408-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: 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/Compass/Compass.tsx`:
- Line 63: The component currently applies className and {...props} to
styles.compassContainer but the new outer root element uses styles.compass,
which breaks public behavior for id, data-*, style, ARIA attrs and root-level
class targeting; update the JSX in the Compass component so that className and
{...props} (and any conditional docked class logic referencing
styles.modifiers.docked) are passed to the outer element using styles.compass
instead of styles.compassContainer, leaving the inner compassContainer as a
purely presentational child.
- Line 63: The Compass component API removed the
backgroundSrcLight/backgroundSrcDark props but tests and demos still pass them;
update the usages in Compass.test.tsx and CompassDemo.tsx to match the new
Compass API by removing those props (or replacing them with the new background
prop if the component now accepts a single background property), ensuring calls
to the Compass component (symbol: Compass) no longer include backgroundSrcLight
or backgroundSrcDark so tests and typings compile.
In `@packages/react-core/src/components/Compass/examples/Compass.md`:
- Line 39: The docs incorrectly claim <CompassHero> supports
backgroundSrcLight/backgroundSrcDark/gradientLight/gradientDark props; update
the Compass.md content to remove references to those props and instead document
the supported approach: set the Compass background globally via the CSS variable
on html (or the theme-level background) and mention that <CompassHero> does not
accept those background props. Edit the paragraph referencing <Compass> and
<CompassHero> to state the correct configuration method and clarify that
customization is global via the html CSS variable rather than per-hero props.
🪄 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: 735252d6-d0eb-4109-9d7c-4f525f16cd9b
📒 Files selected for processing (2)
packages/react-core/src/components/Compass/Compass.tsxpackages/react-core/src/components/Compass/examples/Compass.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-core/src/components/Compass/Compass.tsx`:
- Line 113: The Compass component applies the docked CSS when checking dock !==
undefined which mismatches the dock rendering that uses truthiness; update the
className expression in the Compass JSX (the line that calls css(styles.compass,
...)) to use the same truthy check as rendering (e.g., replace dock !==
undefined && styles.modifiers.docked with dock && styles.modifiers.docked or
!!dock && styles.modifiers.docked) so that the docked style is only applied when
the dock is actually rendered.
🪄 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: 0bf73002-ad28-4ba0-ab13-6c1a8e545b23
⛔ Files ignored due to path filters (1)
packages/react-core/src/components/Compass/__tests__/__snapshots__/Compass.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
packages/react-core/src/components/Compass/Compass.tsxpackages/react-core/src/components/Compass/__tests__/Compass.test.tsx
💤 Files with no reviewable changes (1)
- packages/react-core/src/components/Compass/tests/Compass.test.tsx
Contributor
|
On the docked nav compass demo, can you make sure the panel that wraps the content has these props?
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (1)
398-399:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove deprecated
backgroundSrcDarkandbackgroundSrcLightprops.According to the PR objectives, background props should be removed from the Compass component as "background images are now set globally via CSS and theme." These props are still present in the demo and are inconsistent with the stated goals of this PR.
🔧 Proposed fix
return ( <Compass masthead={mobileMasthead} dock={dockContent} isDockExpanded={isDockExpanded} isDockTextExpanded={isDockTextExpanded} main={mainContent} - backgroundSrcDark="/assets/images/pf-background.svg" - backgroundSrcLight="/assets/images/pf-background.svg" /> );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx` around lines 398 - 399, The CompassDockDemo usage still passes deprecated props backgroundSrcDark and backgroundSrcLight to the Compass component; remove these props from the JSX in CompassDockDemo (search for CompassDockDemo or the Compass component instance in this file) so the component no longer supplies backgroundSrcDark/backgroundSrcLight, relying on the global CSS/theme background instead; ensure no other references or prop types in this file expect those props and run a quick typecheck to catch any residual usages.packages/react-core/src/demos/Compass/examples/CompassDemo.tsx (1)
184-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove deprecated
backgroundSrcDarkandbackgroundSrcLightprops.According to the PR objectives, background props should be removed from the Compass component as "background images are now set globally via CSS and theme." These props are still present in the demo and are inconsistent with the stated goals of this PR.
🔧 Proposed fix
return ( <Compass header={headerContent} sidebarStart={sidebarStartContent} main={mainContent} sidebarEnd={sidebarEndContent} footer={footerContent} - backgroundSrcDark="/assets/images/pf-background.svg" - backgroundSrcLight="/assets/images/pf-background.svg" /> );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-core/src/demos/Compass/examples/CompassDemo.tsx` around lines 184 - 186, Remove the deprecated backgroundSrcDark and backgroundSrcLight props from the Compass component usage in CompassDemo.tsx: locate the JSX for the Compass demo (the component instance that currently includes backgroundSrcDark="/assets/images/pf-background.svg" and backgroundSrcLight="/assets/images/pf-background.svg") and delete those two props so the component relies on global CSS/theme-managed backgrounds instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/react-core/src/demos/Compass/examples/CompassDemo.tsx`:
- Around line 184-186: Remove the deprecated backgroundSrcDark and
backgroundSrcLight props from the Compass component usage in CompassDemo.tsx:
locate the JSX for the Compass demo (the component instance that currently
includes backgroundSrcDark="/assets/images/pf-background.svg" and
backgroundSrcLight="/assets/images/pf-background.svg") and delete those two
props so the component relies on global CSS/theme-managed backgrounds instead.
In `@packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx`:
- Around line 398-399: The CompassDockDemo usage still passes deprecated props
backgroundSrcDark and backgroundSrcLight to the Compass component; remove these
props from the JSX in CompassDockDemo (search for CompassDockDemo or the Compass
component instance in this file) so the component no longer supplies
backgroundSrcDark/backgroundSrcLight, relying on the global CSS/theme background
instead; ensure no other references or prop types in this file expect those
props and run a quick typecheck to catch any residual usages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 093e0ea4-427c-47c2-bb47-a19ae5c1d794
⛔ Files ignored due to path filters (1)
packages/react-core/src/components/Compass/__tests__/__snapshots__/Compass.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
packages/react-core/src/components/Compass/Compass.tsxpackages/react-core/src/components/Compass/CompassMainHeader.tsxpackages/react-core/src/components/Compass/__tests__/Compass.test.tsxpackages/react-core/src/components/Compass/__tests__/CompassMainHeader.test.tsxpackages/react-core/src/components/Compass/examples/Compass.mdpackages/react-core/src/demos/Compass/examples/CompassDemo.tsxpackages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx
💤 Files with no reviewable changes (1)
- packages/react-core/src/components/Compass/tests/Compass.test.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/react-core/src/components/Compass/examples/Compass.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-core/src/components/Compass/Compass.tsx
32 tasks
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
3 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 #12389
Waiting on patternfly/patternfly#8356 to merge first.
Removes background props as those are now set via CSS globally along with theme.
Updates structure set new wrapping div around the outer Drawer.
Summary by CodeRabbit
Refactor
Documentation
Tests
Demos