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

[EuiIcon] Add keyboard glyph #6058

Merged
merged 2 commits into from
Jul 18, 2022
Merged

Conversation

miukimiu
Copy link
Contributor

@miukimiu miukimiu commented Jul 18, 2022

Summary

This PR adds a keyboard glyph to EuiIcon. This icon is required to finish #6036. We could use the icon keyboardShortcut but as we can see from the screenshots on #6036 it's hard to tell what the icon does just looking at it. That's why we decide to create a new icon.

I tried different designs that can be found here: Figma

Frame 15@2x

I run multiple tests in Figma to see how this icon would look in a EuiDataGrid. The prototype can be found here: Figma prototype.

After that, I showed the prototype to @chandlerprall and @constancecchen and both of them picked option 6. I run more tests and this icon didn't work out for non-retina displays (IMO). It gets blurry because the circles are not in the grid and they are not 1px but 1.5px.

Screenshot 2022-07-18 at 17 18 00

Frame 3@2x

So I went back to design and this time I focused on an icon that would work for non-retina displays. From all the tests option 8 worked the best and that's the option I'm adding to EUI.

Screenshot 2022-07-18 at 17 23 00

Final Design

Screenshot 2022-07-18 at 16 51 53

Final design light and dark theme

Frame 1@2x

Deprecation of keyboardShortcut in favor of keyboard

Because the new icon keyboard is more generic we're deprecating the icon keyboardShortcut in favor of the new one. I asked @ryankeairns about his opinion (#6058 (comment)) and we're on the same page.

Screenshot 2022-07-18 at 17 42 58

I searched all Elastic usages of the keyboardShortcut icon and only found the following one: https://github.com/elastic/kibana/blob/f990f31023784f71dde3bb8a74ba52c9c60bbf69/x-pack/plugins/canvas/public/components/help_menu/help_menu.component.tsx#L55

I created a mock in Figma to show how the new icon would look in Kibana. I also replaced one icon usage in our EUI docs.

Before and after@2x

Checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

@ryankeairns
Copy link
Contributor

This is looking good. I would suggest we not keep both and, instead, just go with the new version. Using the name keyboard seems to best align to our preference toward literal names as to not suggest a particular use case.

I don't suspect there are many instances of keyboardShortcut in use in our own product UIs, but this would break others. In either case, the actual fix is about as easy they come.

@miukimiu miukimiu added deprecations Contains deprecations. Add them to the deprecations meta ticket after merge. and removed skip-changelog labels Jul 18, 2022
@miukimiu miukimiu requested a review from cee-chen July 18, 2022 18:09
@miukimiu miukimiu marked this pull request as ready for review July 18, 2022 18:09
@kibanamachine
Copy link

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

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉 Thanks for your awesome work on this Elizabet!! I love the new icon, and think it's super clear!

@miukimiu miukimiu enabled auto-merge (squash) July 18, 2022 18:33
@miukimiu miukimiu merged commit 660a5b3 into elastic:main Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Contains deprecations. Add them to the deprecations meta ticket after merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants