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

[EuiText] Add kbd styles #6049

Merged
merged 4 commits into from
Jul 14, 2022
Merged

Conversation

elizabetdev
Copy link
Contributor

@elizabetdev elizabetdev commented Jul 13, 2022

Summary

Closes #5016.

This PR adds styles for kbd's within EuiText.

kbd within different EuiText sizes:

kbd@2x

Code vs Figma

The implementation differs a little bit from Figma because of the way the font is rendered.

code vs figma@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_6049/

@chandlerprall
Copy link
Contributor

I wonder if we do want to go the component route for this as it would allow controlling the presentation for combinations like Cmd + C

e.g. <EuiKbd keys=['Cmd', 'C']/>

If it isn't something we want to enforce, this adding styles for kbd within EuiText looks good

@kibanamachine
Copy link

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

@elizabetdev
Copy link
Contributor Author

@chandlerprall and @constancecchen,

For this type of implementation a component <EuiKbd keys=['Cmd', 'C']/> would work well:

Screenshot 2022-07-12 at 15 21 22

But for other usages, like documentation, just the HTML tag kbd would work better. I searched for crtl in our repos and I found some *.asciidoc files with it. Just an example:

After the script has run for about 15 seconds, enter CTRL + C to stop it

So I'm assuming that docs sites are the best candidates to use this component/element. @chandlerprall do we need this as a component to be used in docsmobile or is it ok to be a child in EuiText? How would the tag work in mdx?

Regarding the +, I know that @constancecchen is not a fan because of how screen readers read the text. As she mentioned:

People don't tend to say "Control plus alt plus delete", they say "control alt delete" all at once.

So I don't think we should enforce that in a component. But it is a very common practice to use the +:

Concluding. I'm ok with the kbd being only a child of EuiText and I can add a new EuiKeyboard component if it makes easier the usage in docsmobile.

Let me know your thoughts.

@cee-chen
Copy link
Contributor

cee-chen commented Jul 13, 2022

I wonder if we do want to go the component route for this

Haha I just argued against the component route in the spec doc 🙈 To be honest I think it's overkill, like making a component for the <b> tag called <EuiBold> 🤷

Like Elizabet said, I'm also not a huge fan of + between combo keys, I feel like the association is already implicit between two keys next to each other. For that reason I think it makes sense to let folks use <kbd> on a case-by-case basis rather than enforcing a delimiter. That's a relatively loosely held opinion though, I'm definitely willing to be swayed against it.

EDIT: From Michail's keyboard shortcuts research in elastic/kibana#90515, it looks like Google uses + between combos and GitHub doesn't, which I find interesting.

@chandlerprall
Copy link
Contributor

Let's keep this simple then and stick with <kbd>+styles 👍

do we need this as a component to be used in docsmobile or is it ok to be a child in EuiText? How would the tag work in mdx?

The page body is wrapped in an EuiText so the styles should automatically apply. mdx will use the browser's element unless the app overwrites it. Everything should Just Work right out of the box 🥳

@elizabetdev elizabetdev marked this pull request as ready for review July 14, 2022 11:15
- Remove extra newlines
- Remove extra semicolons (already added by logicalCSS() util)
- Refactor 1px border width to `thin` var
- Refactor conditional kdb logic to `euiScaleText` vs a new util
Copy link
Contributor

@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.

🎉

@elizabetdev elizabetdev enabled auto-merge (squash) July 14, 2022 17:00
@kibanamachine
Copy link

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

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

Successfully merging this pull request may close these issues.

Add kbd styling and/or component
4 participants