feat(Compass): remove background props, update structure#12408
Open
kmcfaul wants to merge 2 commits intopatternfly:mainfrom
Open
feat(Compass): remove background props, update structure#12408kmcfaul wants to merge 2 commits intopatternfly:mainfrom
kmcfaul wants to merge 2 commits intopatternfly:mainfrom
Conversation
WalkthroughThe Compass component removes ChangesCompass Background Configuration
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
| - `footer`: Content rendered at the bottom of the page. | ||
|
|
||
| To customize the background image of the `<Compass>` and `<CompassHero>` components, you can use their respective `backgroundSrcLight` and `backgroundSrcDark` props. You can also add and customize a color gradient background for the `<CompassHero>` component by using the `gradientLight` and `gradientDark` props. | ||
| The background image of `<Compass>` is set at a global level alongside the theme. You can customize the background image of `<CompassHero>` by using its `backgroundSrcLight` and `backgroundSrcDark` props, or you may set a gradient using the `gradientLight` and `gradientDark` props. |
There was a problem hiding this comment.
Documentation references unsupported CompassHero background props
Line 39 currently documents backgroundSrcLight/backgroundSrcDark/gradientLight/gradientDark on <CompassHero>, which is misleading and conflicts with the objective to configure Compass background globally via CSS variable on html.
Suggested wording
-The background image of `<Compass>` is set at a global level alongside the theme. You can customize the background image of `<CompassHero>` by using its `backgroundSrcLight` and `backgroundSrcDark` props, or you may set a gradient using the `gradientLight` and `gradientDark` props.
+The background image of `<Compass>` is configured globally via theme CSS variables. For examples, set the background CSS variable on the `html` element’s `style` attribute.📝 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
| The background image of `<Compass>` is set at a global level alongside the theme. You can customize the background image of `<CompassHero>` by using its `backgroundSrcLight` and `backgroundSrcDark` props, or you may set a gradient using the `gradientLight` and `gradientDark` props. | |
| The background image of `<Compass>` is configured globally via theme CSS variables. For example, set the background CSS variable on the `html` element's `style` attribute. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Compass/examples/Compass.md` at 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.
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
|
|
||
| return compassContent; | ||
| return ( | ||
| <div className={css(styles.compass, dock !== undefined && styles.modifiers.docked, className)} {...props}> |
There was a problem hiding this comment.
Align docked-class condition with dock rendering condition.
Line 113 uses dock !== undefined, but dock rendering uses truthiness (dock && ...). With dock={false} / dock={null}, no dock is rendered while docked styling is still applied.
Suggested fix
- <div className={css(styles.compass, dock !== undefined && styles.modifiers.docked, className)} {...props}>
+ <div className={css(styles.compass, Boolean(dock) && styles.modifiers.docked, className)} {...props}>📝 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 className={css(styles.compass, dock !== undefined && styles.modifiers.docked, className)} {...props}> | |
| <div className={css(styles.compass, Boolean(dock) && styles.modifiers.docked, className)} {...props}> |
🤖 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/components/Compass/Compass.tsx` at 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.
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 #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