fix: updated with tokens for menu and menu toggle with unified changes - #8195
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:
WalkthroughUpdated Menu SCSS to add root inline padding tokens, introduce a per-item border-radius token, propagate radii to list pseudo-elements and item containers, and reduce item horizontal padding; adjusted MenuToggle default border-radius tokens for main and plain variants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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 |
|
Preview: https://pf-pr-8195.surge.sh A11y report: https://pf-pr-8195-a11y.surge.sh |
dlabaj
changed the title
|
@thatblindgeye For menu toggle I've changed the radius. I know you have the menu toggle issue this upcoming sprint. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patternfly/components/Menu/menu.scss`:
- Around line 483-485: The CSS uses tokens --#{$menu}__list--PaddingInlineStart
and --#{$menu}__list--PaddingInlineEnd inside the .#{$menu}__list rule but those
custom properties are not defined here, so add definitions (or fallback values)
for --#{$menu}__list--PaddingInlineStart and --#{$menu}__list--PaddingInlineEnd
before they are used (for example define them on .#{$menu} or :root, or supply
var(..., <fallback>) in the padding-inline-start/end declarations); update the
$menu__list token definitions or the .#{$menu} selector so .#{$menu}__list has
valid default values to prevent the declarations from being dropped.
- Around line 487-490: The selector currently matches descendant dividers and
allows nested lists to inherit divider margins; change the selector to only
target direct children by replacing :where(.#{$divider}:is(li)) with a
child-scoped selector (e.g. > .#{$divider}:is(li)) so the margin-block-start/end
CSS using var(--#{$menu}__list--divider--MarginBlockStart) and
var(--#{$menu}__list--divider--MarginBlockEnd) only applies to top-level list
items and not nested drilldown/flyout lists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79ee72e8-2b4b-4e4d-b5dc-4aecc30f4c0b
📒 Files selected for processing (1)
src/patternfly/components/Menu/menu.scss
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 `@src/patternfly/components/Menu/menu.scss`:
- Line 17: The current menu outer radius calculation uses
--#{$menu}--BorderRadius: calc(var(--#{$menu}__item--BorderRadius) +
var(--#{$menu}--PaddingBlockStart)), which can misalign corners when inline and
block paddings differ; update this by either introducing a dedicated
outer-radius token (e.g. --#{$menu}--OuterBorderRadius) and derive
--#{$menu}--BorderRadius from it, or compute it with a robust calc that accounts
for both paddings (e.g. using max(var(--#{$menu}--PaddingBlockStart),
var(--#{$menu}--PaddingInlineStart)) plus var(--#{$menu}__item--BorderRadius));
change the SCSS variable declaration for --#{$menu}--BorderRadius accordingly
and ensure any consuming styles use the new token/name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a66106e4-f69e-492e-81a5-9780372be920
📒 Files selected for processing (1)
src/patternfly/components/Menu/menu.scss
|
Created follow up issue #8201 |
dlabaj
changed the title
|
Added code to resolve issue #8139 as well. |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Some updates needed to the menu component from our group review
- Update menu item hover/focus high contrast border to be on all sides instead of just top/bottom
- Update inline padding for footer, group titles, and breadcrumbs elements to match menu item inline padding. Also just scan the component examples for any other things that look like they're not aligned or have different insets.
- Group titles need to be bold font weight and regular text color (instead of subtle)
- Update menu row gap and divider top/bottom padding/margin to xs spacer
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
For menu toggle we also need to update icons - cog, status, and ellipsis icons should all use RH UI "filled" icons.
Design is going to follow up on the caret icon size to see if brand can adjust the size of the icon - if not, we may need to add a scale to make the icon visually smaller but that can be a follow up.
|
@mcoker Should be all set except for the caret icon as we are waiting on brand. We can do a separate issue if we are going to be held up by this. Let me know and I can put one in. |
…ettings and status icons
|
🎉 This PR is included in version 6.5.0-prerelease.63 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Closes issue #8060 and #8139.
Summary by CodeRabbit