feat(Drawer): Added support for glass#12305
Conversation
WalkthroughThis PR introduces three new beta styling props to Drawer components— ChangesDrawer Styling Props
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)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
|
Preview: https://pf-react-pr-12305.surge.sh A11y report: https://pf-react-pr-12305-a11y.surge.sh |
thatblindgeye
requested review from
kmcfaul,
mcoker and
thatblindgeye
and removed request for
mcoker
|
This PR is waiting for patternfly/patternfly#8267 to be fixed. |
|
@tlabaj just closed patternfly/patternfly#8267, it was done in patternfly/patternfly#8266 which should be in core |
|
@mcoker The modifiers do not seem to be working as expected.
|
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Couple of docs updates but lgtm!
| isResizable?: boolean; | ||
| /** @beta Flag indicating that the drawer panel should disable glass styles. This prop is intended to work with isPill drawers. */ | ||
| hasNoGlass?: boolean; | ||
| /** @beta Flag indicating that the drawer panel should use glass styles. */ |
There was a problem hiding this comment.
Mention applies glass styles in glass theme
| /** @beta Flag indicating that the drawer panel should use plain styles. */ | ||
| isPlain?: boolean; | ||
| /** @beta Flag indicating that plain styles should be disabled when glass styles are used. */ | ||
| isNoPlainOnGlass?: boolean; |
There was a problem hiding this comment.
mention these apply to inline and static drawers.
|
@andrew-ronaldson @lboehling right now we're deprecating the However, "no-background" currently applies to all drawer panels, including overlay and mobile (when the panel takes up the entire layout). The new Is that OK? Or should we update |
|
Looks like an integration test is failing |
|
Confirmed with @andrew-ronaldson that "Overlay shouldn't be transparent imo." So I think we're good to leave |
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
Just needs a fix for the failing integration test, otherwise lgtm.
![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/Drawer/Drawer.tsx`:
- Around line 8-10: Typo in the deprecation JSDoc for
DrawerColorVariant.noBackground: fix the formatting by replacing
"DrawerSection\`instead" with "DrawerSection instead" (or re-wrap correctly as
`DrawerSection` instead) so generated docs/hovers render properly; update the
comment in the Drawer.tsx JSDoc for DrawerColorVariant.noBackground and keep the
rest of the message referencing using the isPlain prop on DrawerPanelContent and
DrawerSection intact.
In `@packages/react-core/src/components/Drawer/DrawerPanelContent.tsx`:
- Around line 65-68: The JSDoc for the colorVariant prop in
DrawerPanelContent.tsx has a missing space in the deprecation sentence; update
the comment where `colorVariant` is documented (in DrawerPanelContent.tsx) to
read "`no-background` is deprecated; use the `isPlain` prop instead." (i.e.,
insert a space between `no-background` and "is") so the API docs render
correctly and improve readability.
In `@packages/react-integration/cypress/integration/drawer.spec.ts`:
- Around line 26-37: The rgbSlashAlpha function misinterprets percentage alphas
(e.g., "50%") because parseFloat returns 50; update rgbSlashAlpha to detect if
the extracted alpha string ends with '%' and, if so, parse the numeric part and
divide by 100 before returning; also trim whitespace, validate the parsed value
and return undefined for NaN or out-of-range values (ensure final alpha is
clamped/validated to 0–1). Reference: rgbSlashAlpha.
🪄 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: 4fdef979-b559-42b3-bdd6-14d2d47d3664
📒 Files selected for processing (10)
packages/react-core/src/components/Drawer/Drawer.tsxpackages/react-core/src/components/Drawer/DrawerPanelContent.tsxpackages/react-core/src/components/Drawer/DrawerSection.tsxpackages/react-core/src/components/Drawer/__tests__/DrawerPanelContent.test.tsxpackages/react-core/src/components/Drawer/__tests__/DrawerSection.test.tsxpackages/react-core/src/components/Drawer/examples/Drawer.mdpackages/react-core/src/demos/examples/PrimaryDetail/PrimaryDetailContentPadding.tsxpackages/react-integration/cypress/downloads/downloads.htmlpackages/react-integration/cypress/integration/drawer.spec.tspackages/react-integration/demo-app-ts/src/components/demos/DrawerDemo/DrawerDemo.tsx
| /** | ||
| * @deprecated `DrawerColorVariant.noBackground` is deprecated. Use the `isPlain` prop on `DrawerPanelContent` and the `DrawerSection`instead. | ||
| */ |
There was a problem hiding this comment.
Fix deprecation JSDoc typo in public API docs.
The deprecation text has a formatting typo (DrawerSection\instead`) that will show up in generated docs and editor hovers.
Proposed fix
- * `@deprecated` `DrawerColorVariant.noBackground` is deprecated. Use the `isPlain` prop on `DrawerPanelContent` and the `DrawerSection`instead.
+ * `@deprecated` `DrawerColorVariant.noBackground` is deprecated. Use the `isPlain` prop on `DrawerPanelContent` and `DrawerSection` instead.📝 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.
| /** | |
| * @deprecated `DrawerColorVariant.noBackground` is deprecated. Use the `isPlain` prop on `DrawerPanelContent` and the `DrawerSection`instead. | |
| */ | |
| /** | |
| * `@deprecated` `DrawerColorVariant.noBackground` is deprecated. Use the `isPlain` prop on `DrawerPanelContent` and `DrawerSection` instead. | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Drawer/Drawer.tsx` around lines 8 - 10,
Typo in the deprecation JSDoc for DrawerColorVariant.noBackground: fix the
formatting by replacing "DrawerSection\`instead" with "DrawerSection instead"
(or re-wrap correctly as `DrawerSection` instead) so generated docs/hovers
render properly; update the comment in the Drawer.tsx JSDoc for
DrawerColorVariant.noBackground and keep the rest of the message referencing
using the isPlain prop on DrawerPanelContent and DrawerSection intact.
| /** | ||
| * Color variant of the background of the drawer panel. | ||
| * The `no-background`is deprecated; use the `isPlain` prop instead. | ||
| */ |
There was a problem hiding this comment.
Clean up deprecation sentence spacing in colorVariant docs.
There’s a missing space in ``no-backgroundis, which hurts API doc readability.
Proposed fix
- * The `no-background`is deprecated; use the `isPlain` prop instead.
+ * The `no-background` value is deprecated; use the `isPlain` prop instead.📝 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.
| /** | |
| * Color variant of the background of the drawer panel. | |
| * The `no-background`is deprecated; use the `isPlain` prop instead. | |
| */ | |
| /** | |
| * Color variant of the background of the drawer panel. | |
| * The `no-background` value is deprecated; use the `isPlain` prop instead. | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Drawer/DrawerPanelContent.tsx` around
lines 65 - 68, The JSDoc for the colorVariant prop in DrawerPanelContent.tsx has
a missing space in the deprecation sentence; update the comment where
`colorVariant` is documented (in DrawerPanelContent.tsx) to read
"`no-background` is deprecated; use the `isPlain` prop instead." (i.e., insert a
space between `no-background` and "is") so the API docs render correctly and
improve readability.
| const rgbSlashAlpha = (color: string): number | undefined => { | ||
| if (!color.startsWith('rgb(')) { | ||
| return undefined; | ||
| } | ||
| const slash = color.indexOf('/'); | ||
| const close = color.lastIndexOf(')'); | ||
| if (slash === -1 || close === -1 || slash >= close) { | ||
| return undefined; | ||
| } | ||
| const a = parseFloat(color.slice(slash + 1, close).trim()); | ||
| return Number.isNaN(a) ? undefined : a; | ||
| }; |
There was a problem hiding this comment.
Handle percentage alpha correctly in rgbSlashAlpha.
parseFloat('50%') returns 50, so semi-transparent colors written as percentages can be treated as opaque by mistake.
Proposed fix
const rgbSlashAlpha = (color: string): number | undefined => {
if (!color.startsWith('rgb(')) {
return undefined;
}
const slash = color.indexOf('/');
const close = color.lastIndexOf(')');
if (slash === -1 || close === -1 || slash >= close) {
return undefined;
}
- const a = parseFloat(color.slice(slash + 1, close).trim());
- return Number.isNaN(a) ? undefined : a;
+ const rawAlpha = color.slice(slash + 1, close).trim();
+ const parsedAlpha = parseFloat(rawAlpha);
+ if (Number.isNaN(parsedAlpha)) {
+ return undefined;
+ }
+ return rawAlpha.endsWith('%') ? parsedAlpha / 100 : parsedAlpha;
};📝 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.
| const rgbSlashAlpha = (color: string): number | undefined => { | |
| if (!color.startsWith('rgb(')) { | |
| return undefined; | |
| } | |
| const slash = color.indexOf('/'); | |
| const close = color.lastIndexOf(')'); | |
| if (slash === -1 || close === -1 || slash >= close) { | |
| return undefined; | |
| } | |
| const a = parseFloat(color.slice(slash + 1, close).trim()); | |
| return Number.isNaN(a) ? undefined : a; | |
| }; | |
| const rgbSlashAlpha = (color: string): number | undefined => { | |
| if (!color.startsWith('rgb(')) { | |
| return undefined; | |
| } | |
| const slash = color.indexOf('/'); | |
| const close = color.lastIndexOf(')'); | |
| if (slash === -1 || close === -1 || slash >= close) { | |
| return undefined; | |
| } | |
| const rawAlpha = color.slice(slash + 1, close).trim(); | |
| const parsedAlpha = parseFloat(rawAlpha); | |
| if (Number.isNaN(parsedAlpha)) { | |
| return undefined; | |
| } | |
| return rawAlpha.endsWith('%') ? parsedAlpha / 100 : parsedAlpha; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-integration/cypress/integration/drawer.spec.ts` around lines
26 - 37, The rgbSlashAlpha function misinterprets percentage alphas (e.g.,
"50%") because parseFloat returns 50; update rgbSlashAlpha to detect if the
extracted alpha string ends with '%' and, if so, parse the numeric part and
divide by 100 before returning; also trim whitespace, validate the parsed value
and return undefined for NaN or out-of-range values (ensure final alpha is
clamped/validated to 0–1). Reference: rgbSlashAlpha.
6 participants
What: Closes #12273
Changes to drawer:
Changes to DrawerPanelContent:
isGlass(@beta, default false) — appliespf-m-glassisPlain(@beta, default false) — appliespf-m-plainisNoPlainOnGlass(@beta, default false) — appliespf-m-no-plainDrawerColorVariant.noBackgroundmarked @deprecated; migrate to isPlain for plain styling (still applies pf-m-no-background so existing snapshots and consumers stay stable).isGlass,isPlain,isNoPlainOnGlass, and clarified test name for deprecated no-backgroundChanges for DrawerSection:
added
isPlain(@beta, default false) — appliespf-m-plainDrawerColorVariant.noBackgroundmarked @deprecated; migrate to isPlain for plain styling (still appliespf-m-no-backgroundso existing snapshots and consumers stay stable).Added new test for
isPlainUpdated PrimaryDetailContentPadding demo: DrawerPanelContent uses i
sPlain; removed invalid DrawerContentcolorVariant="no-background"(not supported on DrawerContent)Summary by CodeRabbit
New Features
isGlass,isPlain,isNoPlainOnGlass) to DrawerPanelContent for enhanced visual customization.isPlainbeta prop to DrawerSection.Deprecations
no-backgroundstyle variant is deprecated; use the newisPlainprop instead.