Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiKeyPadMenu] Add selection state #4000

Closed
legrego opened this issue Sep 3, 2020 · 9 comments · Fixed by #4950
Closed

[EuiKeyPadMenu] Add selection state #4000

legrego opened this issue Sep 3, 2020 · 9 comments · Fixed by #4950

Comments

@legrego
Copy link
Member

legrego commented Sep 3, 2020

Would it be possible to add a selection state to EuiKeyPadMenu?

I'm working through a redesign of Kibana's spaces management page, and one of the suggestions that @MichaelMarcialis proposed was a toggleable set of these components to choose which type of Space Avatar the user wants (image based or text based).

Not a blocker for my work to continue, but wanted to raise this as a possible enhancement.

image

Link to Figma: https://www.figma.com/proto/dBta1q3JgFe3Cfhw5h76Oq/%5BAwaiting-Dev%5D-Spaces-%26-Roles-Solution-Grouping?node-id=20%3A2087&scaling=min-zoom

@myasonik
Copy link
Contributor

myasonik commented Sep 3, 2020

I do have some a11y concerns about this... Either EUI would have to add checkbox/radio item semantics to the keypadmenu (which wouldn't strictly be hard technically, but EUI has rightfully avoided adding multiple semantic meanings to the same visual component) or Kibana would have to create the correct semantics on top of an EUI component that wasn't designed for it (which often can lead to hacky code and/or misuse).

Neither issue is insurmountable at all, but wanted to present them earlier rather than later.

@MichaelMarcialis
Copy link
Contributor

Thanks for writing up this issue, @legrego. And thanks for the thoughtful feedback, @myasonik.

For further context, in some of my more recent designs (such as the example Larry provided), I was hoping to promote the expansion of the current capabilities of the EuiKeyPadMenu component. In the use case presented above, my choice in proposing a selectable EuiKeyPadMenuItem was based on:

  • A need for a selectable element that included both a visual and textual component.
  • Being smaller and more succinct than the EuiCardSelect element (which is much larger and also requires a description).
  • Emphasizing the visual (iconography) aspects more heavily to quickly illustrate available options to users without requiring them to read.
  • A desire to account for future scalability (as additional options may be added over time) better than current components, such as EuiButtonGroup.

Engineering-wise, what I'm proposing would likely ask that:

  • The presence of EuiKeyPadMenuItem borders be optionally shown at all times (perhaps via prop), rather than only showing them on hover/focus.
  • Options for applying some degree of gutter spacing between EuiKeyPadMenuItem components. Perhaps it may be also worth considering the current fixed width of 96px currently in place on this component as well.
  • Account for the possibility of at least a single (though there could also be arguments made for multiple) select states. As Michail mentioned in his comments above though, this will alter both the markup rendered and thus the semantic meaning of the component (which may be frowned upon).

Larry has already provided one example above that I've worked up in Figma, but I'd be happy to provide others or to discuss further.

@cchaos
Copy link
Contributor

cchaos commented Sep 4, 2020

I think this would be a nice addition to the component. I've been eyeing some other examples of nice radio/selection groups and thinking of ways we could enhance our own.

As @myasonik was mentioning this is quite trickier on the implementation side when it comes to ensuring proper accessibility (keyboard and screen-reader output). If it were just a quick change of style, ✅ done! But we've already been in a summer-long conversion of EuiButtonGroup for better accessibility which will have the same implications as this component.

Mostly I'm just setting up expectations that this will take more effort and time than it seems. We'll probably need to completely re-think how we handle visual selections and allowing any component to render in the proper context.

@cchaos cchaos changed the title Add selection state to EuiKeyPadMenu [EuiKeyPadMenu] Add selection state Sep 20, 2020
@cchaos cchaos added ⚠️ needs spec Should be groomed by the EUI team every week to ensure a spec is added accessibility assign:anyone and removed prop request labels Sep 20, 2020
@ryankeairns
Copy link
Contributor

This same design has been proposed for the custom header/footer feature: elastic/kibana#17298 (comment)

It's not officially targeted to a specific release, but is a possibility for 7.12

@cchaos cchaos self-assigned this Nov 12, 2020
@cchaos
Copy link
Contributor

cchaos commented Nov 12, 2020

I think the best way to handle this one is to create a "checkable" version of EuiKeyPadMenu (similar to EuiCheckableCard) which will display a visible checkbox or radio (depending on multi or single select). However, this input element will only be present when interacting (hover/focus) or when selected. So the unselected item will continue to look similar as it does today.

Using the inputs gives us 2 things:

  1. We aren't signifying selection with color alone
  2. Gives us reasonable focus states in combination with keyboard navigation

Here's the mocks that I'll use to build off of. The top row is Amsterdam vs default theme below. Each row shows, unselected, selected, and hover states.

Screen Shot 2020-11-12 at 11 37 53 AM

@cchaos cchaos removed accessibility assign:anyone ⚠️ needs spec Should be groomed by the EUI team every week to ensure a spec is added labels Nov 12, 2020
@cchaos cchaos added this to the Elastic Stack 7.11 milestone Nov 12, 2020
@github-actions
Copy link

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@legrego
Copy link
Member Author

legrego commented May 12, 2021

@elastic/kibana-security is still interested in this!

@cchaos
Copy link
Contributor

cchaos commented May 12, 2021

Yeah I've got a WIP local branch that's gone a bit stale on this one. I'll try to pick it back up before or right after 7.14 FF.

@legrego What's your timeline for needing it?

@legrego
Copy link
Member Author

legrego commented May 12, 2021

Yeah I've got a WIP local branch that's gone a bit stale on this one. I'll try to pick it back up before or right after 7.14 FF.

@legrego What's your timeline for needing it?

That's great news! We don't have an explicit timeline -- we would like this for some a11y and ux improvements to the Spaces Management page, but this isn't blocking any critical work for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants