feat(menu): add high-contrast border - #7725
Conversation
|
Preview: https://pf-pr-7725.surge.sh A11y report: https://pf-pr-7725-a11y.surge.sh |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Left a comment about the pseudo border. Also @lboehling should there be a border for selected state? Single selected and multi-selected
| --#{$menu}--BackgroundColor: var(--pf-t--global--background--color--floating--default); | ||
| --#{$menu}--BoxShadow: var(--pf-t--global--box-shadow--md); | ||
| --#{$menu}--Color: var(--pf-t--global--text--color--regular); | ||
| --#{$menu}--BorderWidth: var(--pf-t--global--high-contrast--border--width--control--default); |
There was a problem hiding this comment.
should this use the box token?
| --#{$menu}--BorderWidth: var(--pf-t--global--high-contrast--border--width--control--default); | |
| --#{$menu}--BorderWidth: var(--pf-t--global--border--width--box--default); |
| &::after { | ||
| position: absolute; | ||
| inset: 0; | ||
| pointer-events: none; | ||
| content: ""; | ||
| border: var(--#{$menu}--BorderWidth) solid var(--#{$menu}--BorderColor); | ||
| border-radius: inherit; | ||
| } |
There was a problem hiding this comment.
What's the lift to fix the drilldown stuff if we move this to a regular border? I'd expect menu to have issues with children that overlap the border. I know we have a feature request to make group titles sticky that would mess up this border unless we figured out a z-index thing for this pseudo element.
Screen.Recording.2025-08-11.at.12.54.40.PM.mov
|
Updated to put borders onto the menu element rather than the pseudo. This increases the size of the menu, so the React calculation of the menu height will need to take this into consideration (until then, you'll see the last menu item on a drilled in menu without a bottom border, as it's being cut off) This also changes the menu to use Regression test shows 1px height change where I had to increase the manually specified size to accommodate the new borders. There's also a shift in the modified-width drilldown expansion caret, which I think results from the menu border but I didn't increase the manual width (I can if you like and then I think the regression would pass). This will require a fix in React to account for the borders when calculating the height. (in MenuContent.tsx) |
No border needed for selected state since the menu items have other indications for its selected state (check mark) that meet the color contrast requirements. |
I could be wrong, but this looks like it may be due to the borders on the nested menu, which don't look to be disabled. When I disable the borders on nested menus, looks like the menus position correctly without modifying I wonder if that might mean the height adjustments aren't required, too?
Looking at the examples, the PR has |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
Also left a separate comment about the drilldown updates.
| --#{$menu}__list-item--BorderWidth: var(--pf-t--global--high-contrast--border--width--control--default); | ||
| --#{$menu}__list-item--BorderColor: transparent; | ||
| --#{$menu}__list-item--hover--BorderColor: var(--pf-t--global--border--color--high-contrast); |
There was a problem hiding this comment.
Should we update this to set the color by default and use the action--plain tokens?
| --#{$menu}__list-item--BorderWidth: var(--pf-t--global--high-contrast--border--width--control--default); | |
| --#{$menu}__list-item--BorderColor: transparent; | |
| --#{$menu}__list-item--hover--BorderColor: var(--pf-t--global--border--color--high-contrast); | |
| --#{$menu}__list-item--BorderWidth: var(--pf-t--global--border--width--action--plain--default); | |
| --#{$menu}__list-item--BorderColor: var(--pf-t--global--border--color--high-contrast); | |
| --#{$menu}__list-item--hover--BorderWidth: var(--pf-t--global--border--width--action--plain--hover); |
fa24c9e to
bc87666
Compare
4 participants
Fixes #7600
Added border to existing pseudoelement on the menu item
Added border to a new ::after on the menu - used the pseudo because otherwise the drilldown calculation of the size was off by the amount of the border.
Note - The nav with drilldown menu items have a slight border radius that touches the menu's border. I'm not sure if this is right or not.
