feat(Menu): consumed Penta updates by thatblindgeye · Pull Request #10089

feat(Menu): consumed Penta updates by thatblindgeye · Pull Request #10089 · patternfly/patternfly-react

Conversation

What: Closes #9985, closes #10076

There's some styling issues on hover for the menu item action. This is because in Core there's no pf-v5-c-menu__item-action-icon wrapper around the icon, while in React there is.

Additional issues:

		aria-label={ariaLabel}
		onClick={onClickButton}
		ref={innerRef}
		role="menuitem"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should help resolve an aXe error that is related to #9968. This also matches the Core markup.

That said, it'd be worth looking into an alternative to making each button inside a menu a role="menuitem". My concern would be whether it's be totally clear that a user can navigate up/down and left/right inside a menu, or whether the assumption would be that there's only items in a vertical scope.

Comment on lines +98 to +99

		/** @hide Sets the role of the button. Should only be used when the button is a descendant of a menu or tablist. */
		role?: string;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant to expose this publicly since it's a very specific use-case. I originally had just imported the button styles into MenuItemAction and applied the necessary class names to a native element, so we could do that instead.

This could be worth exposing to the consumer once we can create some demos on when/how to properly use the prop.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably fine to remove this and leave it as an internal prop until we need it to be exposed. But hiding it also works for me.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, we could move the spread props in button to come after all the other props (currently they're spread first). Wasn't sure if there was a particular reason for that or if it's safe to just move that, but that was the reason for adding the hidden role prop.

	return (
	<Component
	{...props}
	{...(isAriaDisabled ? preventedEvents : null)}
	aria-disabled={isDisabled \|\| isAriaDisabled}
	aria-label={ariaLabel}
	className={css(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was linked to issues

Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes lgtm from the react side, but I'm noticing for the drilldown menu that the transition animation only is applying to the first level of menu. @mcoker @mattnolting Any ideas about this? I don't think it's on the react-side since the drilldown logic hasn't been updated.

Changes lgtm from the react side, but I'm noticing for the drilldown menu that the transition animation only is applying to the first level of menu. @mcoker @mattnolting Any ideas about this? I don't think it's on the react-side since the drilldown logic hasn't been updated.

@kmcfaul We lost a couple variable values. Here's the fix: patternfly/patternfly#6340

Eric Olkowski added 4 commits

February 26, 2024 11:39

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, I have a few questions

Eric Olkowski added 2 commits

February 26, 2024 13:18

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfecto! 💯

		tabIndex={tabIndex !== null ? tabIndex : getDefaultTabIdx()}
		type={isButtonElement \|\| isInlineSpan ? type : null}
		role={isInlineSpan ? 'button' : null}
		role={isInlineSpan ? 'button' : role}