feat(Menu): consumed Penta updates #10089
Conversation
| aria-label={ariaLabel} | ||
| onClick={onClickButton} | ||
| ref={innerRef} | ||
| role="menuitem" |
There was a problem hiding this comment.
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.
| /** @hide Sets the role of the button. Should only be used when the button is a descendant of a menu or tablist. */ | ||
| role?: string; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
patternfly-react/packages/react-core/src/components/Button/Button.tsx
Lines 146 to 152 in 08a6b1a
There was a problem hiding this comment.
@kmcfaul updated
|
Preview: https://patternfly-react-pr-10089.surge.sh A11y report: https://patternfly-react-pr-10089-a11y.surge.sh |
thatblindgeye
force-pushed
the
menuTogglePenta
branch
from
3dadc39 to
826a839
Compare
There was a problem hiding this comment.
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 |
thatblindgeye
force-pushed
the
menuTogglePenta
branch
from
826a839 to
1b08af1
Compare
thatblindgeye
requested review from
mattnolting,
nicolethoen,
a team,
lboehling,
kaylachumley and
wise-king-sullyman
and removed request for
a team
|
@andrew-ronaldson Should the favorite button be on the right or left?
|
| @@ -182,7 +183,7 @@ const ButtonBase: React.FunctionComponent<ButtonProps> = ({ | |||
| disabled={isButtonElement ? isDisabled : null} | |||
| tabIndex={tabIndex !== null ? tabIndex : getDefaultTabIdx()} | |||
| type={isButtonElement || isInlineSpan ? type : null} | |||
| role={isInlineSpan ? 'button' : null} | |||
| role={isInlineSpan ? 'button' : role} | |||
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
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-iconwrapper around the icon, while in React there is.Additional issues: