🚚 Move Keyboard Shortcuts

[simurai]

This moves the keyboard shortcuts component from github/github to Primer CSS.

TODO

• Add styles
• Add documentation See 🚚 Move Keyboard Shortcuts #904 (comment)
• Improve/refactor the CSS without breaking anything

TODO on .com

• Remove .badmono https://github.com/github/github/pull/124900
• Remove .kb-shortcut-dialog https://github.com/github/github/pull/124900
• 🔥 Remove keyboard-shortcuts.scss.

---

Tracking https://github.com/github/design-systems/issues/684

[vercel]

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://primer-css-git-kbd.primer.now.sh

[simurai]

Can the overrides for .markdown-body be removed? It's pretty much the same, but has slightly different colors:

HTML	Screenshot
<kbd>cmd f</kbd>
<span class="markdown-body"><kbd>cmd f</kbd></span>

Maybe we should keep it for people that only use the Markdown bundle with

@import "@primer/css/markdown/index.scss";

We could sync up the colors, but not sure if there is some history behind them?

[shawnbot]

Can the overrides for .markdown-body be removed? It's pretty much the same, but has slightly different colors:

I would vote yes. The markdown-specific styles don't add much, IMO.

[shawnbot]

This is great. I'm honestly not sure what the "right" thing is to do about the Markdown-specific styles, though. Our options appear to be:

1. Delete the .markdown-body kbd styles altogether. This assumes that anyone using the markdown bundle is also using core, but I guess if this were to break for somebody we could just tell them to add an import?
2. Add an @import "../labels/keyboard-shortcuts.scss"; inside the .markdown-body {} block so that the styles are duplicated, but still available in the standalone markdown bundle.

Am I missing something?

[shawnbot]

I also think it might make sense to call this file kbd.scss rather than keyboard-shortcuts.scss.

[simurai]

I also think it might make sense to call this file kbd.scss rather than keyboard-shortcuts.scss.

Would make sense since we style the element directly. Was also thinking if it should be part of base.scss? But then it can't be found when clicking on "View source" in the docs.

ok, yeah.. let's change it to kbd.scss.

---

Markdown-specific styles

Hmmm.. with option 1, looks like we would have to import it for mobile and embedded gists?

But that's only for github/github. There might be lots more..? Also Primer CSS users outside of GitHub. So maybe try option 2. At least we can avoid maintaining it in two places. ✨

[simurai]

Add an @import "../labels/keyboard-shortcuts.scss"; inside the .markdown-body {} block so that the styles are duplicated, but still available in the standalone markdown bundle.

Hmm.. now I'm not sure anymore... 😆

It feels a bit weird having the markdown bundle import from the labels bundle.

Option 3:

Treat <kbd> styling the same as we currently treat <hr>s. They have default styling in base.scss that gets overridden in .markdown-body. I'll already make the change to get a feel for it.

Then in the future we can consider turning <kbd> and <hr> (and maybe more?) into "real" components, that get their own class name, like .KeyboardShortcut and .Divider or similar.

[shawnbot]

I like how you handled this, @simurai. I think we can remove most (all?) of the styles from .markdown-body if it's in base, as much of the Markdown styles rely on base anyway.

[simurai]

Hmm.. I think we have to keep the kbd styles in .markdown-body because on mobile, we use a custom base.scss and for embedded gists we don't import any base styles.

[simurai]

Ok, the markdown kbd has now the exact same values as in base: 8d2952f. It will slightly change in color, but not too drastic. It also has the mono font stack added.

Made a "reminder" issue to consider removing kbd from markdown: #919

[shawnbot]

Okay I'm sorry @simurai, but now that they're exactly the same I think we should move the styles into src/base/kbd.scss and use @import for both instances. This gives it the stature of a component, provides us with a single source of truth, and prevents the appearance from diverging.

[simurai]

should move the styles into src/base/kbd.scss and use @import for both instances.

ok, yeah.. it felt wrong importing from /labels into the markdown bundle. Importing from /base should be fine. 👍

[shawnbot]

Huzzah, this is great! I suppose it doesn't really need docs of its own for now, right?

[shawnbot]

It looks like in its current state this also closes #919, right?

[simurai]

I suppose it doesn't really need docs of its own for now, right?

Maybe not, unless we wanna document all the base styles.

It looks like in its current state this also closes #919, right?

Hmmm.. well, now that it's part of a major release, we could remove it from the markdown bundle. 🤔 But it's also nice being able to only import the markdown bundle and having support for <kbd>s already included. So I'm fine with closing #919 as is.

[shawnbot]

@simurai I opened #935 for a minor release before 14.0.0, so feel free to merge this there if you want to get it out sooner. 🚀