fix(drawer): glass drawer placement and tokens update#8286
Conversation
|
Preview: https://pf-pr-8286.surge.sh A11y report: https://pf-pr-8286-a11y.surge.sh |
|
The pill variant (inline and overlay) should have the option to use a splitter as well! Thinking it may be good to add that since its styled slightly different then the splitter in a standard sqaured off drawer |
kaylachumley
left a comment
There was a problem hiding this comment.
left a comment about the splitter with pill drawers!
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughRetargeted Drawer panel/border tokens, added an expanded-pill box-shadow variable and a resizable-pill Drawer example, removed a glass-theme masthead bottom margin, and introduced glass-specific page sidebar/main margin variables. (27 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
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)
src/patternfly/components/Page/page.scss (1)
259-278:⚠️ Potential issue | 🟠 MajorFix the glass theme selector so Stylelint passes.
Stylelint is flagging Line 259 for
nesting-selector-no-missing-scoping-root. Use the existing theme-scoped@at-rootpattern here before wiring the new variables.Proposed fix
- :where(.pf-v6-theme-glass) & { + `@at-root` :where(.pf-v6-theme-glass) & { --#{$page}--BackgroundColor: var(--#{$page}--BackgroundColor--glass); --#{$page}__sidebar--Width--base: var(--#{$page}__sidebar--Width--base--glass); --#{$page}__sidebar--MarginBlockStart: var(--#{$page}__sidebar--MarginBlockStart--glass);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/components/Page/page.scss` around lines 259 - 278, Stylelint complains because the glass theme block uses a nested selector without the repository's theme-scoped `@at-root` pattern; wrap the existing :where(.pf-v6-theme-glass) & block in the same `@at-root` theme-scoping used elsewhere so the selector becomes theme-scoped at root before you set the variables (the block that wires variables like --#{$page}--BackgroundColor, --#{$page}__sidebar--Width--base, --#{$page}__sidebar-main--BackgroundColor, --#{$page}__sidebar-main--BackdropFilter, --#{$page}__main-container--MarginBlockStart, etc.). Locate the :where(.pf-v6-theme-glass) & block in page.scss and apply the project’s `@at-root` .pf-v6-theme-glass & pattern around it so Stylelint no longer flags nesting-selector-no-missing-scoping-root.src/patternfly/components/Drawer/drawer.scss (1)
354-359:⚠️ Potential issue | 🟡 MinorRoute the pill shadow through the existing shadow variable.
Line 359 applies
box-shadowdirectly, which bypasses later resets for inline/static drawers and.pf-m-no-borderpanels. Set--#{$drawer}--m-expanded__panel--BoxShadowinstead so existing cascade rules can still override it.🎨 Proposed fix
> .#{$drawer}__main > .#{$drawer}__panel { + --#{$drawer}--m-expanded__panel--BoxShadow: var(--#{$drawer}--m-pill--m-expanded__panel--BoxShadow); + border-block-start-width: var(--#{$drawer}--m-pill__panel--BorderBlockStartWidth); border-block-end-width: var(--#{$drawer}--m-pill__panel--BorderBlockEndWidth); border-inline-start-width: var(--#{$drawer}--m-pill__panel--BorderInlineStartWidth); border-inline-end-width: var(--#{$drawer}--m-pill__panel--BorderInlineEndWidth); - box-shadow: var(--#{$drawer}--m-pill--m-expanded__panel--BoxShadow); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/components/Drawer/drawer.scss` around lines 354 - 359, The selector .#{$drawer}__main > .#{$drawer}__panel is currently setting box-shadow directly; change it to assign the expanded-panel shadow to the cascadeable CSS variable instead (set --#{$drawer}--m-expanded__panel--BoxShadow using the pill-expanded value rather than writing box-shadow), so later rules (inline/static drawers and .pf-m-no-border) can override it and the cascade remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/patternfly/components/Drawer/drawer.scss`:
- Around line 354-359: The selector .#{$drawer}__main > .#{$drawer}__panel is
currently setting box-shadow directly; change it to assign the expanded-panel
shadow to the cascadeable CSS variable instead (set
--#{$drawer}--m-expanded__panel--BoxShadow using the pill-expanded value rather
than writing box-shadow), so later rules (inline/static drawers and
.pf-m-no-border) can override it and the cascade remains intact.
In `@src/patternfly/components/Page/page.scss`:
- Around line 259-278: Stylelint complains because the glass theme block uses a
nested selector without the repository's theme-scoped `@at-root` pattern; wrap the
existing :where(.pf-v6-theme-glass) & block in the same `@at-root` theme-scoping
used elsewhere so the selector becomes theme-scoped at root before you set the
variables (the block that wires variables like --#{$page}--BackgroundColor,
--#{$page}__sidebar--Width--base, --#{$page}__sidebar-main--BackgroundColor,
--#{$page}__sidebar-main--BackdropFilter,
--#{$page}__main-container--MarginBlockStart, etc.). Locate the
:where(.pf-v6-theme-glass) & block in page.scss and apply the project’s `@at-root`
.pf-v6-theme-glass & pattern around it so Stylelint no longer flags
nesting-selector-no-missing-scoping-root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: eee80095-ce9d-423f-a2e1-1c04e67bea9e
📒 Files selected for processing (4)
src/patternfly/components/Drawer/drawer.scsssrc/patternfly/components/Drawer/examples/Drawer.mdsrc/patternfly/components/Masthead/masthead.scsssrc/patternfly/components/Page/page.scss
💤 Files with no reviewable changes (1)
- src/patternfly/components/Masthead/masthead.scss
mcoker
left a comment
There was a problem hiding this comment.
Just a small docs nit to add a unique ID
Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com>
|
🎉 This PR is included in version 6.5.0-prerelease.74 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #8234
Drawer
Secondary panel: background uses background--color--floating--secondary--default instead of secondary--default.
Pill variant: border color uses border--color--subtle. Expanded pill panel gets box-shadow--md for clearer separation.
Page / masthead (glass)
Removes masthead margin-block-end in the glass theme so the band no longer owns that gap.
Adds matching top margin on the sidebar and main container via new glass tokens (MarginBlockStart = page chrome inset), so spacing stays consistent while living on the page chrome instead of the masthead.
Summary by CodeRabbit
Style
Documentation