chore(menu): add tokens by mattnolting · Pull Request #6167 · patternfly/patternfly

mattnolting · 2023-12-19T21:49:20Z

closes #6162
closes #6218

patternfly-build · 2023-12-19T21:50:06Z

Preview: https://patternfly-pr-6167.surge.sh

A11y report: https://patternfly-pr-6167-a11y.surge.sh

srambach · 2024-01-09T14:13:33Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

  --#{$breadcrumb}__item-divider--FontSize: var( --pf-t--global--font--size--body--sm);

  // Breadcrumb link
+  --#{$breadcrumb}__link--PaddingInlineStart: revert; // use a mutable value for alignment control


Were changed to this file meant to be in this PR?

Looks like this is causing the focus styles to look like this:

Disabling this inline start padding causes the link text to be more centered horizontally in the focus ring

srambach

Couple of code questions and a couple of visual questions -

Should the split button toggles be pill shaped? Also, this one doesn't have the border radius all around on the disabled version.

I think the highlighting isn't working right on disabled menu items with actions/favs?

2024-01-09_08-40-53.mp4

srambach · 2024-01-09T14:20:15Z

src/patternfly/components/Menu/menu.scss

-  --#{$menu}--m-drilldown__content--TransitionDuration--transform: var(--#{$pf-global}--TransitionDuration);
+  --#{$menu}__item-action--hover--BackgroundColor: var(--pf-t--global--background--color--action--plain--hover);
+  --#{$menu}__item-action--focus--BackgroundColor: var(--pf-t--global--background--color--action--plain--focus);
+  --#{$menu}__item-action--m-selected--BackgroundColor: var();


Did you mean to do this?

thatblindgeye · 2024-01-12T21:14:16Z

src/patternfly/components/Menu/menu.scss

  --#{$menu}__search--PaddingLeft: var(--#{$pf-global}--spacer--md);
+  --#{$menu}__search--PaddingRight: var(--#{$pf-global}--spacer--md);


It looks like the Figma design shows no inline padding. @lboehling can you confirm? I'm looking at the Filter Search Input design.

I assumed that to be errant.

src/patternfly/components/Menu/menu.scss

thatblindgeye · 2024-01-16T13:42:11Z

src/patternfly/components/Menu/menu.scss

+  --#{$menu}__list-item--m-danger--Color: var(--pf-t--global--text--color--status--danger--default);
+  --#{$menu}__list-item--m-load__item--Color: var(--pf-t--global--text--color--link--default);
+  --#{$menu}__list-item--has--menu-action--PaddingRight: var(--pf-t--global--spacer--sm);
+  --#{$menu}__list-item--child--not-active--BackgroundColor: var(--pf-t--global--background--color--action--plain--default);


Related to above.

This prevents parent siblings from changing background on hover

Just wanted to check that this is still needed? Tried removing where this var is being applied locally and at least for the Flyout and Drilldown examples it didn't seem to make a difference

src/patternfly/components/Menu/menu.scss

andrew-ronaldson · 2024-01-18T15:36:55Z

When I click on the menu examples the borders disappear?

mattnolting · 2024-01-18T16:56:52Z

When I click on the menu examples the borders disappear?

@andrew-ronaldson Same thing happens on pf6. That's part of the menu toggle, separate from menu. I'm updating menu toggle now :)

srambach

👨‍🍳 fully cooked!

mcoker

Left some comments, though in v5, hovering/focusing anything in a menu list item (the main item and/or any actions in it) would trigger the background change on the list item, and I see that was removed here. In checking with @lboehling, it looks like we want to keep the behavior from v5, and that impacts a lot of my feedback. Let's regroup on Monday and see what the best path forward is to getting menu updated for alpha.

mcoker · 2024-02-02T01:41:01Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

+  &:first-child {
+    margin-inline-start: calc(var(--#{$breadcrumb}__link--PaddingInlineStart) * -1); // left align first item in list with list bounds while retaining link padding
+  }


Why did we add padding to the links? There isn't always a __link in an __item, so one of the examples looks like this. I think it would be good to avoid the negative margin if we can.

Currently, breadcrumbs are misaligned when link is present in the first child.

Removing padding from the first link resolves this issue:

Oh I was asking why the padding was added in the first place. It doesn't exist in v6 currently. This last example is misaligned but that's because the browser/agent styles are adding padding to the breadcrumb link when it's a button. IMO that's just a bug we overlooked since we don't want the default button element styles - we'd just remove that (set breadcrumb__link to padding: 0).

This is what I'm seeing in this PR currently

src/patternfly/components/Menu/menu.scss

mcoker · 2024-02-02T02:46:27Z

src/patternfly/components/Menu/menu.scss

+  --#{$menu}__item-action--PaddingBlock: var(--pf-t--global--spacer--sm);
+  --#{$menu}__item-action--PaddingInline: var(--pf-t--global--spacer--md);
+  --#{$menu}__item-action--FontSize: var(--pf-t--global--font--size--body--default);


doesn't look like any of these are used?

mcoker · 2024-02-02T21:23:22Z

src/patternfly/components/Menu/menu.scss

+
+// - Menu disabled
+.#{$menu}__item,
+.#{$menu}__action {


Doesn't look like .#{$menu}__action is a thing?

mcoker · 2024-02-02T21:47:47Z

src/patternfly/components/Menu/menu.scss

    justify-content: center;
-    padding-block-start: var(--#{$menu}__list-item--m-loading--PaddingTop);
    overflow: hidden; // prevents spinner rotation from overflowing
+    pointer-events: none;


I'm assuming disabling pointer-events keeps the backgrounds and hover/interaction styles from showing up? It's still focusable and tabbable. Though if it shouldn't be interacted with, it shouldn't have a button inside.

@tlabaj @thatblindgeye is the loading item button something that can interact with, or do you think that's something we might support? As in (just as an example), someone could click on it to stop loading whatever it's doing? Also a side note, at least in this example, the "view more" link has tabindex="-1" so I can't tab to it - is that a bug? https://www.patternfly.org/components/menus/menu/react/with-view-more/

@mcoker If you are referring to the spinner, it is currently a span with not onClick callback. At this pont I would not anticipate a use case where we would click on it.

I believe the tabindex="-1" is correct. We do not typically tab through our menu items. We have been navigating them with the up and down arrow keys. @thatblindgeye please correct me if I am wrong.

That's correct regarding the tabindex. We do update the tabindex to 0 in React when an item actually receives focus via the arrow keys (just so the previously focused item will receive focus again upon entering the Menu, unless the menu was closed), but otherwise should be -1.

The/one of the only times we'd need to tab to content in a Menu is if there's actions in a footer/outside the actual menu item list.

If you are referring to the spinner, it is currently a span with not onClick callback. At this pont I would not anticipate a use case where we would click on it.

@tlabaj I'm referring to the button.pf-v5-c-menu__item that wraps it - you can see it in the screenshot below

@mcoker I see. Even thought the spinner is rendered in a button, the onClick is only called when the "View more" button is clicked and not the spinner. If we wanted to allow for that we would need to modify the code. I would not consider ding that right now unless we had a request for it. At that point I would probably get UX's opinion on the interaction.

mcoker · 2024-02-03T00:36:44Z

src/patternfly/components/Menu/menu-item-action.hbs

-  {{/if}}
+<div class="{{pfv}}menu__item-action
+  {{setModifiers
+    menu-list-item--IsAriaDisabled='pf-m-aria-disabled'


Do you need this on both the __item-action container and the action inside?

mcoker · 2024-02-03T00:42:14Z

src/patternfly/components/Menu/menu.scss

+  padding-block-start: var(--#{$menu}--PaddingBlockStart);
+  padding-block-end: var(--#{$menu}--PaddingBlockEnd);
+  overflow: hidden;
+  color: var(--#{$menu}--Color);


Is setting color needed?

mcoker · 2024-02-03T00:43:06Z

src/patternfly/components/Menu/menu.scss

  color: var(--#{$menu}__list-item--Color);
  background-color: var(--#{$menu}__list-item--BackgroundColor);


Are either of these needed?

background-color is needed, color is not

mattnolting · 2024-02-07T17:58:02Z

@lboehling

Updates:

Favorited button hover color
List item, dark mode hover mix-blend-mode
Removed button role from loading <li>

srambach · 2024-02-08T15:30:31Z

It doesn't look like selected menu items are showing the check when selected? https://patternfly-pr-6167.surge.sh/components/menus/menu/#option-multi-select

lboehling · 2024-02-09T16:33:15Z

Noticed a few more things!

Flyout menus look like they're getting the action plain hover across the entire menu bkg, these should have the same bkg (floating) as the main menu.

Drilldown levels 2-X aren't getting the blend mode applied to make them lighter (1st level drilldowns all look correct though)

the scrolling menu examples have a weird rounded top corner and inset on the first item of the list

Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com> Update src/patternfly/components/Menu/menu.scss Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com> Update src/patternfly/components/Menu/menu.scss Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com> Update src/patternfly/components/Menu/menu.scss Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com> Update src/patternfly/components/Menu/menu.scss Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com> Update src/patternfly/components/Menu/menu.scss Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com>

mcoker

LGTM! I'll open a couple of follow-up issues for higher and lower priority updates.

mcoker · 2024-02-14T15:57:46Z

src/patternfly/components/Menu/menu.scss

+  --#{$menu}__list-item--BackgroundColor: var(--pf-t--global--background--color--action--plain--default);
+  --#{$menu}__list-item--hover--BackgroundColor: var(--pf-t--global--background--color--action--plain--hover);
+  --#{$menu}__list-item--MixBlendMode: normal;
+  --#{$menu}__list-item--MixBlendMode--hover: var(--pf-t--global--background--color--action--plain--hover--blend);


nit

Suggested change

--#{$menu}__list-item--MixBlendMode--hover: var(--pf-t--global--background--color--action--plain--hover--blend);

--#{$menu}__list-item--hover--MixBlendMode: var(--pf-t--global--background--color--action--plain--hover--blend);

mcoker · 2024-02-14T16:19:24Z

src/patternfly/components/Menu/menu.scss

+    padding-inline-end: var(--#{$menu}__list-item--has--menu-action--PaddingInlineEnd);
+  }
+
+  &:has(> :hover) {


Can this not just be &:hover? This also needs to apply on focus, :focus-within should do it.

mcoker · 2024-02-14T16:21:50Z

src/patternfly/components/Menu/menu.scss


-  &.pf-m-selected {
-    .#{$menu}__item-select-icon {
+  &:is(:hover, :focus, :active, .pf-m-selected) {


is :active needed? If it isn't needed, I'd prefer remove it and we can leave :active as it is across the board unless there is a design/code reason for it.

mcoker · 2024-02-14T16:26:22Z

src/patternfly/components/Menu/menu.scss

-  .#{$check} {
-    --pf-v5-c-check__input--TranslateY: none;
+  // expand clickable area to full width/height of list__item
+  input::before {


This shouldn't be needed, and I'm pretty sure a pseudo element on an <input> isn't in the spec (this doesn't work in FF).

For checkbox items the __item is a <label>, which should make the whole thing clickable, but it isn't working currently because the label has an invalid for attribute for the input inside of it. Pretty sure we just need to take the for attribute off entirely, it isn't needed.

mcoker · 2024-02-14T16:28:19Z

src/patternfly/components/Menu/menu.scss

+  &.pf-m-favorited,
+  &.pf-m-favorited:hover,
+  &.pf-m-favorited .#{$button} {


Could this not just target the button? eg, remove the first two selectors.

Suggested change

&.pf-m-favorited,

&.pf-m-favorited:hover,

&.pf-m-favorited .#{$button} {

&.pf-m-favorited .#{$button} {

mcoker · 2024-02-14T16:36:09Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

+  --#{$breadcrumb}__link--PaddingInlineStart: var(--pf-t--global--spacer--sm); // use a mutable value for alignment control
+  --#{$breadcrumb}__link--PaddingInlineEnd: var(--pf-t--global--spacer--sm); // use a mutable value for alignment control


I'd still like to understand why padding was added. v5 has inline padding in the buttons example but it's a bug and shouldn't be there.

mcoker · 2024-02-14T16:44:48Z

src/patternfly/components/Menu/menu.scss

+.#{$menu},
+.#{$menu}__list,
+.#{$menu}__group {
+  @include pf-v5-hidden-visible(grid);
+}
+
+// - Menu shared display flex
+.#{$menu}__content,
+.#{$menu}__list-item,
+.#{$menu}__item,
+.#{$menu}__item-main,
+.#{$menu}__breadcrumb,
+.#{$menu}__item-check,
+.#{$menu}__item-action,
+.#{$menu}__item-action-icon {
+  @include pf-v5-hidden-visible(flex);
+}


Why are all of these using hidden-visible? In v5, only __list, __item, and __group use it. If we aren't trying to support the pf-m-hidden/visible[-on-breakpoint] on all of these elements, we should remove this. If we are trying to support that, we should talk about why.

mcoker · 2024-02-14T16:46:36Z

src/patternfly/components/Menu/menu.scss

+
+// - Menu item disabled - Menu action disabled
+.#{$menu}__list-item,
+.#{$menu}__item-action {


Why is item-action included here? From glancing at it, it looks like the only thing that would apply to item-action is pointer-events but the action that goes inside of item-action (the button component) should handle all of that for us.

mcoker · 2024-02-14T17:13:11Z

src/patternfly/components/Menu/menu.scss

+
+  &:has(> :hover) {
+    --#{$menu}__list-item--BackgroundColor: var(--#{$menu}__list-item--hover--BackgroundColor);
+    --#{$menu}__list-item--MixBlendMode: var(--#{$menu}__list-item--MixBlendMode--hover);


For some reason, this is creating a different color between variations of the menu. Basic vs scrollable and the drilldown/breadcrumb examples below as examples:

mcoker · 2024-02-14T17:25:29Z

src/patternfly/components/Menu/menu.scss

+  --#{$menu}__item-action--FontSize: var(--pf-t--global--font--size--body--default);
+  --#{$menu}__item-action--icon--size: var(--#{$menu}__item-action--FontSize, var(--pf-t--global--icon--size--md));
+  --#{$menu}__item-action--Color: var(--pf-t--global--icon--color--subtle);
+  --#{$menu}__item-action--m-favorited--Color: var(--pf-t--global--icon--color--favorite--clicked);
+  --#{$menu}__item-action--button--MinWidth: calc(var(--#{$menu}__item-action--icon--size) + var(--pf-t--global--spacer--sm) * 2);


I think this is the only one that's needed now?

Suggested change

--#{$menu}__item-action--FontSize: var(--pf-t--global--font--size--body--default);

--#{$menu}__item-action--icon--size: var(--#{$menu}__item-action--FontSize, var(--pf-t--global--icon--size--md));

--#{$menu}__item-action--Color: var(--pf-t--global--icon--color--subtle);

--#{$menu}__item-action--m-favorited--Color: var(--pf-t--global--icon--color--favorite--clicked);

--#{$menu}__item-action--button--MinWidth: calc(var(--#{$menu}__item-action--icon--size) + var(--pf-t--global--spacer--sm) * 2);

--#{$menu}__item-action--m-favorited--Color: var(--pf-t--global--icon--color--favorite--clicked);

patternfly-build · 2024-02-15T15:31:35Z

🎉 This PR is included in version 6.0.0-alpha.84 🎉

The release is available on:

Your semantic-release bot 📦🚀

mattnolting force-pushed the chore-menu-tokens branch from 2f1c88a to f99bad6 Compare December 19, 2023 23:38

mcoker changed the title ~~Chore menu tokens~~ chore(menu): add tokens Dec 20, 2023

mattnolting force-pushed the chore-menu-tokens branch 2 times, most recently from b6f701e to 0f068ae Compare December 21, 2023 06:02

wise-king-sullyman linked an issue Jan 2, 2024 that may be closed by this pull request

Consume tokens: Menu #6162

Closed

mattnolting force-pushed the chore-menu-tokens branch 6 times, most recently from a12cea2 to c2b0b21 Compare January 4, 2024 20:41

mattnolting marked this pull request as ready for review January 5, 2024 15:02

mattnolting requested review from mcoker, srambach and thatblindgeye January 5, 2024 15:02

srambach reviewed Jan 9, 2024

View reviewed changes

mattnolting force-pushed the chore-menu-tokens branch 3 times, most recently from 46ea5d3 to 0376b24 Compare January 11, 2024 03:57

thatblindgeye reviewed Jan 16, 2024

View reviewed changes

mattnolting force-pushed the chore-menu-tokens branch from 0376b24 to b339af2 Compare January 16, 2024 16:30

mattnolting requested a review from thatblindgeye January 16, 2024 16:31

mattnolting force-pushed the chore-menu-tokens branch from b339af2 to 418b137 Compare January 16, 2024 17:35

This was referenced Jan 16, 2024

Consume Penta tokens: Menu patternfly/patternfly-react#9985

Closed

Consume Penta tokens: Menu toggle patternfly/patternfly-react#9986

Closed

srambach approved these changes Feb 1, 2024

View reviewed changes

mcoker reviewed Feb 3, 2024

View reviewed changes

mattnolting force-pushed the chore-menu-tokens branch 3 times, most recently from b2f975c to 1a1b619 Compare February 6, 2024 21:37

mattnolting force-pushed the chore-menu-tokens branch from 1a1b619 to 5dfe29c Compare February 8, 2024 02:35

mattnolting force-pushed the chore-menu-tokens branch from 5dfe29c to 0aa4da7 Compare February 8, 2024 15:54

mattnolting and others added 8 commits February 9, 2024 11:52

chore(menu): @thatblindgeye updates!

ba7b13e

chore(menu): @lboehling updates!

d95bbef

chore(menu): lint fix

d584f79

chore(menu): @mcoker updates

5bd5647

chore(menu): updated flyout examples

e7a98cd

chore(menu): updated vars

cb86281

update

d15c8a5

mattnolting force-pushed the chore-menu-tokens branch from 0aa4da7 to d15c8a5 Compare February 9, 2024 21:27

mcoker approved these changes Feb 14, 2024

View reviewed changes

mcoker merged commit ea761a1 into patternfly:v6 Feb 14, 2024

This was referenced Feb 14, 2024

Consume tokens: Menu #6162

Closed

Workspace - add support for "and" operator in tests #6218

Closed

Menu - follow ups for alpha #6319

Closed

Menu follow ups #6320

Closed

patternfly-build added the released on @alpha label Feb 15, 2024

This was referenced Feb 16, 2024

fix(menu): updates for alpha #6322

Merged

Bug - Nav - flyout menus are broken #6327

Closed

patternfly-build mentioned this pull request Apr 16, 2025

The automated release is failing 🚨 #7464

Closed

		--#{$menu}__search--PaddingLeft: var(--#{$pf-global}--spacer--md);
		--#{$menu}__search--PaddingRight: var(--#{$pf-global}--spacer--md);

		color: var(--#{$menu}__list-item--Color);
		background-color: var(--#{$menu}__list-item--BackgroundColor);

	--#{$menu}__list-item--MixBlendMode--hover: var(--pf-t--global--background--color--action--plain--hover--blend);
	--#{$menu}__list-item--hover--MixBlendMode: var(--pf-t--global--background--color--action--plain--hover--blend);

		--#{$breadcrumb}__link--PaddingInlineStart: var(--pf-t--global--spacer--sm); // use a mutable value for alignment control
		--#{$breadcrumb}__link--PaddingInlineEnd: var(--pf-t--global--spacer--sm); // use a mutable value for alignment control

Conversation

mattnolting commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srambach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andrew-ronaldson commented Jan 18, 2024

Uh oh!

mattnolting commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srambach left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattnolting commented Feb 7, 2024

Uh oh!

srambach commented Feb 8, 2024

Uh oh!

lboehling commented Feb 9, 2024

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

mattnolting commented Dec 19, 2023 •

edited

Loading

patternfly-build commented Dec 19, 2023 •

edited

Loading

mattnolting commented Jan 18, 2024 •

edited

Loading