[EuiKeyPadMenuItem] Added `checkable` option & style updates #4950

cchaos · 2021-07-13T19:24:01Z

Style updates

Before, both the default and Amsterdam themes had the same styling: a simple bordered-on-hover-only item.

Now, the Amsterdam version is more aligned to the designs from Figma and I've added an isSelected prop for indicating current items. I also increased the contrast of the text as the subdued color was too subdued for such large (possible primary) menu items and when the icons inherit their color.

AFTER:

Checkable

Closes #4000

Adds `checkable = 'multi' | 'single'` to the EuiKeyPadMenuItem component

When multi, will display as a checkbox and when single will display as a radio (on hover & selected). It changes the consumer's required interaction prop from onClick to onChange and uses isSelected to chance the checked status.

Adds `checkable` prop to EuiKeyPadMenu for wrapping the group as a `<fieldset>`

Originally I thought to just showcase it being wrapped in an EuiFormRow, but it comes with too much baggage. Therefore, it just allows consumers to pass either:

true: Just renders the <fieldset> and requires consumers to still pass in their own labelling via any method they like
{ legend | ariaLegend }: Where ariaLegend is just a string applied to arial-label of the fieldset for a visibly hidden legend. They can further customize the <legend> with legendProps.

This creates the proper accessibility hook-ups.

Other

I also fixed up some styling associated with the beta badge to strictly use the new EuiBetaBadge props.

Checklist

…d a11y props/elements

Also added a helper mixin for `euiAnimationsAllowed`

kibanamachine · 2021-07-13T19:45:57Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4950/

cchaos · 2021-07-13T22:13:01Z

src/components/key_pad_menu/key_pad_menu_item.tsx

+    // @ts-ignore HELP!
    <Element


@chandlerprall Since you self-assigned... 😉

I get this error when I remove this ignore:

Type '{ children: Element; ref: ((instance: HTMLAnchorElement | null) => void) | RefObject<HTMLAnchorElement> | ((instance: HTMLButtonElement | null) => void) | RefObject<...> | null | undefined; ... 252 more ...; onTransitionEndCapture?: ((event: TransitionEvent<...>) => void) | undefined; }' is not assignable to type 'IntrinsicAttributes'. Property 'children' does not exist on type 'IntrinsicAttributes'.

I've pushed a change to help address this. There's still a fallback to as Ref<any> but I don't think it's worth trying to completely satisfy TS there.

😢 I don't think your change got pushed

cchaos · 2021-07-13T22:15:47Z

src/components/key_pad_menu/_key_pad_menu.scss

-.euiKeyPadMenuItem {
-  @include euiFont; /* 1 */
-  // Disable indentation for transition legibility
-  // sass-lint:disable-block indentation
-  display: block;
-  padding: $euiSizeXS;


FYI: I moved all these item styles to their own file.

kibanamachine · 2021-07-13T22:20:22Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4950/

kibanamachine · 2021-07-13T22:53:48Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4950/

myasonik · 2021-07-14T14:10:05Z

src/components/key_pad_menu/key_pad_menu.tsx

+    /**
+     * Renders the the group as a `fieldset` with a `legend` to label the items.
+     */
+    legend?: ReactNode;


Thoughts on moving checkable up to here and making legend required if checkable='multi'?

I think it would seem weird if some of the items in a single menu are radio inputs and others checkable

A radiogroup should always have a legend

Yeah I tried that but it doesn't work very well the way we make users manually supply each item vs passing in an object. Mainly the problem lies in the complexity of TS and knowing if the iterative child IS an EuiKeyPadMenuItem or not and can then accept the checkable prop as a pass-through.

Not to say it's not possible, just beyond what I can do for now. You are more than happy to take a stab at it.

Not sure I'll get to it right now but will keep this in the queue.

Another option that would be easier is to require checkable in both places. It doesn't have to do much right now (even just to help with the legend bit right now seems good enough) but will also prevent a breaking change in the future when/if we get to the point that we can remove it from each item within.

It's a little duplicative but helps the upgrade path in the future of where we might want this to go.

Oh, could use CSS to "block" passing in children with the wrong checkable type if we wanted to. Something like adding an obnoxious red border around mismatched items if they don't match the container.

Well, I actually tested what happens is a legend is provided but the items aren't checkable, just <button>s and the SR does provide the legend as a grouping title which I thought was a plus outside of requiring it only if the items are checkable.

myasonik · 2021-07-14T14:13:02Z

src/components/key_pad_menu/key_pad_menu.tsx

-  return (
+  return legend ? (
+    <fieldset className={classes} {...rest}>
+      <EuiFormLabel type="legend">{legend}</EuiFormLabel>


I can see folks avoiding adding a legend because they don't like how it's visually presented.

Should we add the ability to visibly hide the <legend> and/or to supply an aria-label/aria-labelledby to the <fieldset> instead to give that visual flexibility or do we want to enforce a consistent style here?

What approach would you like to see for that? I'd like to stay away from just a boolean isLegendVisible type of prop if possible because it can be misunderstood.

🤔 It's hard/low-value if checkable isn't on the KeyPadMeny (because we shouldn't force a legend if it's not checkable=multi)...

But if it is, I don't have a specific approach in mind. The isLegendVisisble flag is probably what I would have reached for first. Why don't you like it?

Another option is to have two props: legend and ariaLegend (where ariaLegend could also be called screenReaderLegend, visuallyHiddenLegend, etc)

The Charts team has basically taken the aria-* prefix to mean anything screen reader only...

I like that approach of using ariaProp names to indicate visibility. I worry that it could confuse consumers that it is a real aria- attribute, but probably not a big deal. We can comment as such.

I don't "like" isLegendVisible, because boolean props tend not to be scalable. It's fine for very true booleans like isDisabled, but we don't know in the future what other legend options may be provided. I'll think on a solution.

Ok I updated this to where EuiKeyPadMenu now accepts a singular checkable prop which can be either:

true: Just renders the <fieldset> and requires consumers to still pass in their own labelling via any method they like

{ legend | ariaLegend }: Where ariaLegend is just a string applied to arial-label of the fieldset

They can further customize the <legend> with legendProps

chandlerprall

LGTM, pulled & tested functionality & TypeScript configuration locally; pushed a TS change; have one small recommendation for documenting isSelected

src/components/key_pad_menu/key_pad_menu_item.tsx

chandlerprall · 2021-07-14T21:02:23Z

src/components/key_pad_menu/key_pad_menu_item.tsx

+    // @ts-ignore HELP!
    <Element


I've pushed a change to help address this. There's still a fallback to as Ref<any> but I don't think it's worth trying to completely satisfy TS there.

elizabetdev

LGTM! 🎉

Tested in Chrome, Safari, Edge, and Firefox. I also looked at the code. 👍🏽

kibanamachine · 2021-07-16T14:35:27Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4950/

…e` prop

kibanamachine · 2021-07-16T20:22:43Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4950/

myasonik

Thanks for the changes - looks good!

I'm taking a look at the typescript issue right now, hopefully I can clobber something together.

myasonik · 2021-07-19T20:41:30Z

src/components/key_pad_menu/key_pad_menu_item.tsx

+type EuiKeyPadMenuItemPropsForAnchor = PropsForAnchor<
+  EuiKeyPadMenuItemCommonProps,
+  {
+    buttonRef?: Ref<HTMLAnchorElement>;


Thoughts on changing this (throughout the file) to itemRef? buttonRef seems confusing when it can also be an anchor...

Do you think it's a necessary breaking change?

myasonik · 2021-07-19T20:43:42Z

@thompsongl Maybe you can take a pass at fixing the TS issue? I'm not getting anywhere 😕

Otherwise we can wait till Chandler's back

cchaos · 2021-07-19T21:12:35Z

Otherwise we can wait till Chandler's back

Yes, there is no rush on this PR at all.

thompsongl · 2021-07-19T21:36:43Z

There's still a fallback to as Ref<any> but I don't think it's worth trying to completely satisfy TS there.

I arrived at what I assume is pretty much the same place as Chandler. Changes pushed.

kibanamachine · 2021-07-19T22:00:16Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4950/

kibanamachine · 2021-07-19T23:13:27Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4950/

cchaos · 2021-07-20T14:10:52Z

Thanks all for the help and reviews!

cchaos added 8 commits November 19, 2020 17:14

[EuiKeyPadMenuItem] Added isSelected and fixed a few interactive an…

b5dc3a6

…d a11y props/elements

Adding styles for Amsterdam version

5788b04

Also added a helper mixin for `euiAnimationsAllowed`

Very manual addition of checkable

8b2e8d5

Fixing up styles

cf088b1

Merge remote-tracking branch upstream/master into new/checkable_keypad

c3374ec

Fixing mixin

7dbc54e

Added label prop to menu and cleaned up styles and docs

d142992

Cleaning up the beta badge and more docs

a38cbb1

Fix contrast

8755850

cchaos changed the title ~~[EuiKeyPadMenuItem] Added checkable option~~ [EuiKeyPadMenuItem] Added checkable option & style updates Jul 13, 2021

chandlerprall self-requested a review July 13, 2021 21:35

cchaos commented Jul 13, 2021

View reviewed changes

cchaos added 2 commits July 13, 2021 18:27

Merge remote-tracking branch 'upstream/master' into new/checkable_keypad

6970f6f

cl

39352a3

cchaos requested a review from elizabetdev July 13, 2021 22:30

cchaos marked this pull request as ready for review July 13, 2021 22:30

myasonik reviewed Jul 14, 2021

View reviewed changes

chandlerprall approved these changes Jul 14, 2021

View reviewed changes

elizabetdev approved these changes Jul 16, 2021

View reviewed changes

Merge branch 'master' into new/checkable_keypad

640827a

cchaos and others added 3 commits July 16, 2021 15:53

Changing [EuiKeyPadMenu] fieldset options to be added via a `checkabl…

54414e2

…e` prop

cl

23489c9

Merge branch 'master' into new/checkable_keypad

a47fb2c

cchaos requested a review from myasonik July 16, 2021 19:59

myasonik approved these changes Jul 19, 2021

View reviewed changes

myasonik reviewed Jul 19, 2021

View reviewed changes

Element types

13886bc

Merge branch 'master' into new/checkable_keypad

076afc0

cchaos merged commit 30ca57f into elastic:master Jul 20, 2021

cchaos deleted the new/checkable_keypad branch July 20, 2021 14:11

thompsongl mentioned this pull request Jul 20, 2021

Upgrade EUI to v36.0.0 elastic/kibana#106334

Merged

[EuiKeyPadMenuItem] Added checkable option & style updates #4950

[EuiKeyPadMenuItem] Added checkable option & style updates #4950

Uh oh!

Conversation

cchaos commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Style updates

Checkable

Adds checkable = 'multi' | 'single' to the EuiKeyPadMenuItem component

Adds checkable prop to EuiKeyPadMenu for wrapping the group as a <fieldset>

Other

Checklist

Uh oh!

kibanamachine commented Jul 13, 2021

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!

kibanamachine commented Jul 13, 2021

Uh oh!

kibanamachine commented Jul 13, 2021

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!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elizabetdev left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 16, 2021

Uh oh!

kibanamachine commented Jul 16, 2021

Uh oh!

myasonik 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!

myasonik commented Jul 19, 2021

Uh oh!

cchaos commented Jul 19, 2021

Uh oh!

thompsongl commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Jul 19, 2021

Uh oh!

kibanamachine commented Jul 19, 2021

Uh oh!

cchaos commented Jul 20, 2021

Uh oh!

[EuiKeyPadMenuItem] Added `checkable` option & style updates #4950

[EuiKeyPadMenuItem] Added `checkable` option & style updates #4950

cchaos commented Jul 13, 2021 •

edited

Loading

Adds `checkable = 'multi' | 'single'` to the EuiKeyPadMenuItem component

Adds `checkable` prop to EuiKeyPadMenu for wrapping the group as a `<fieldset>`

thompsongl commented Jul 19, 2021 •

edited

Loading