Skip to content

[EuiText] Add kbd styles#6049

Merged
elizabetdev merged 4 commits intoelastic:mainfrom
elizabetdev:add-kbd-styles
Jul 14, 2022
Merged

[EuiText] Add kbd styles#6049
elizabetdev merged 4 commits intoelastic:mainfrom
elizabetdev:add-kbd-styles

Conversation

@elizabetdev
Copy link
Copy Markdown
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

@elizabetdev elizabetdev added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Jul 13, 2022
@kibanamachine
Copy link
Copy Markdown

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

@chandlerprall
Copy link
Copy Markdown
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
Copy Markdown

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

@elizabetdev
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 requested a review from cee-chen July 14, 2022 11:09
@elizabetdev elizabetdev removed the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label Jul 14, 2022
@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
Copy Markdown
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
Copy Markdown

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