Add `KeybindingHint` component #4750

iansan5653 · 2024-07-18T17:28:08Z

Closes https://github.com/github/primer/issues/3169

Adds a new component called KeybindingHint. This is upstreamed from our internal codebase, where it is used in production in several locations. As part of the upstreaming, I've taken the liberty of renaming the component from KeyboardKey to KeybindingHint to better indicate its purpose and flexibility. I've also refactored the code, splitting it into smaller files.

Changelog

New

New KeybindingHint component for indicating an available keybinding

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2024-07-18T17:28:12Z

🦋 Changeset detected

Latest commit: c378c1e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-07-18T17:31:55Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	96.09 KB (-0.11% 🔽)
packages/react/dist/browser.umd.js	96.39 KB (-0.07% 🔽)

iansan5653 · 2024-07-18T17:58:50Z

packages/react/src/hooks/useIsMacOS.ts

+/**
+ * SSR-safe hook for determining if the current platform is MacOS. When rendering
+ * server-side, will default to non-MacOS and then re-render in an effect if the
+ * client turns out to be a MacOS device.
+ */
+export function useIsMacOS() {
+  // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
+  const [isMacOS, setIsMacOS] = useState(() => (window !== undefined ? ssrUnsafeIsMacOS() : false))
+
+  useEffect(() => setIsMacOS(ssrUnsafeIsMacOS()), [])
+
+  return isMacOS
+}


In the current usage of this component internally, we call isMacOS directly at the top level of the file. This is obviously not good for SSR-compatibility, so I've introduced this new hook:

In non-SSR, it causes no extra renders and returns isMacOS directly

In SSR, it initially returns false, then causes a re-render after hydration if the value would change to true (if the client is using a Mac)

I exposed this hook publicly so that we can also use it elsewhere when actually binding keyboard shortcuts.

keithamus

I will look at this more closely this week but for now...

packages/react/src/KeybindingHint/KeybindingHint.tsx

keithamus · 2024-07-18T18:11:07Z

packages/react/src/KeybindingHint/components/Chord.tsx

+
+export const Chord = ({keys, format = 'condensed', variant = 'normal'}: KeybindingHintProps) => (
+  <Text
+    sx={{


Maybe we can make this use a CSS module? @joshblack

I'm definitely open to this. I didn't see any other CSS modules in the package so I figured I'd keep it consistent for now, but happy to make the change.

The work we do for CSS modules is still WIP, hopefully should come in soon though 🤞🏻

I wonder if we can write the styles migration in mind? 🤔 What I mean is that for example instead of styling the color conditionally

color: variant === 'onEmphasis' ? 'fg.onEmphasis' : 'fg.muted',

Can we use data attributes so that we can load the styles all at once rather than rendering conditionally and can create a good path for moving them into a css file?

I'll tag @joshblack and @langermank to see what they think if this is worth doing and get a second opinion.

Do you have any prior art for using data attributes for this? I'm not sure I've seen that approach before.

FWIW this should be pretty easy to migrate - we will just introduce an additional class like onEmphasis for the variant.

Ah sorry I didn't provide any example. We use data attributes heavily for the Button

FWIW this should be pretty easy to migrate - we will just introduce an additional class like onEmphasis for the variant.

Okay then. That sounds good. Let's keep the styles as is and we can migrate later.

.vscode/settings.json

…into add-keybinding-hint

packages/react/src/KeybindingHint/KeybindingHint.stories.tsx

mperrotti · 2024-08-13T12:59:51Z

packages/react/src/KeybindingHint/KeybindingHint.tsx

+    <Sequence {...props} />
+  </Kbd>
+))
+KeybindingHint.displayName = 'KeybindingHint'


Why are we setting the displayName property when it already matches the name of the function?

The engine can infer a function name for simple named declarations like function Component() {} and const Component = ()=> {}.

But in this case we are declaring an anonymous function without a name, then sending it through memo and assigning it to a named constant. There's no way for the engine to infer a name for the function so the component would be anonymous without a displayName. This is actually required by the linter.

broccolinisoup

Thanks for addressing all these comments quickly 🙏🏻 Left a few more comments - looking forward to hearing your thoughts!!

broccolinisoup · 2024-08-14T06:21:34Z

packages/react/src/KeybindingHint/components/Chord.tsx

+
+export const Chord = ({keys, format = 'condensed', variant = 'normal'}: KeybindingHintProps) => (
+  <Text
+    sx={{


Ah sorry I didn't provide any example. We use data attributes heavily for the Button

FWIW this should be pretty easy to migrate - we will just introduce an additional class like onEmphasis for the variant.

Okay then. That sounds good. Let's keep the styles as is and we can migrate later.

broccolinisoup · 2024-08-14T08:43:17Z

packages/react/src/KeybindingHint/components/Key.tsx

+      <VisuallyHidden>{accessibleKeyName(name, isMacOS)}</VisuallyHidden>
+      <span aria-hidden>{format === 'condensed' ? condensedKeyName(name, isMacOS) : fullKeyName(name, isMacOS)}</span>


Wonderful! I have played with it as well and it works great ✨

I have one comment though regarding using this in Tooltip:

I quickly implemented it to see how that would work and looks in Tooltip and I think we need to work through the colours if this is how we are going to use this component in Tooltip

tagging @mperrotti here for design 👀

broccolinisoup · 2024-08-14T08:46:45Z

packages/react/src/KeybindingHint/components/Key.tsx

+      <VisuallyHidden>{accessibleKeyName(name, isMacOS)}</VisuallyHidden>
+      <span aria-hidden>{format === 'condensed' ? condensedKeyName(name, isMacOS) : fullKeyName(name, isMacOS)}</span>


Regarding using getAccessibleKeybindingHintString , is there a way to handle determining isMacOS under the hood so that we don't worry about it every time when we use this function?

Let me know if there is anything I am missing here

broccolinisoup · 2024-08-14T08:48:24Z

packages/react/src/KeybindingHint/key-names.ts

+ * readers from expressing punctuation in speech, ie, reading a long pause instead of the
+ * word "period".
+ */
+export const accessibleKeyName = (key: string, isMacOS: boolean) =>


Thank you! I'll share this with the team to see if we need to cross check this again with the Accessibility team. Thanks for your patience 🙏🏻

siddharthkp · 2024-08-16T09:22:55Z

packages/react/src/__tests__/KeybindingHint.test.tsx

@@ -0,0 +1,97 @@
+import React from 'react'


(not blocking) You can move these inside packages/react/src/KeyboardHint/__tests__, that's the preferred location

siddharthkp · 2024-08-16T09:26:14Z

docs/src/@primer/gatsby-theme-doctocat/nav.yml

@@ -82,6 +82,8 @@
      url: /Heading
    - title: IconButton
      url: /IconButton
+    - title: KeybindingHint
+      url: /KeybindingHint


Should this be under drafts as well in the nav?

(non blocking because these docs are going away soon anyways in favor of https://primer.style/components)

broccolinisoup

I assume we don't have any usages at dotcom since it is a new component and the overall PR and the API looks good to me.! 🚀

We can always address later if any issues come up or if there is anything we want to improve.

iansan5653 · 2024-08-16T14:11:05Z

I assume we don't have any usages at dotcom

To be clear, this is upstreaming a component that's in dotcom, so we do have a few existing usages (under the name KeyboardKey). I'll handle migrating them after this is shipped.

iansan5653 added 4 commits July 18, 2024 16:41

Add KeybindingHint component

9104157

Split file and refactor a bit

66dc77e

Split components out into individual files

04904cd

Update comments

cec4e50

iansan5653 self-assigned this Jul 18, 2024

Create useIsMacOS hook for SSR support

8a37a6f

iansan5653 force-pushed the add-keybinding-hint branch from 8777a58 to 8a37a6f Compare July 18, 2024 17:55

iansan5653 commented Jul 18, 2024

View reviewed changes

Add changelog

b0e1e7a

iansan5653 force-pushed the add-keybinding-hint branch from badc0a7 to b0e1e7a Compare July 18, 2024 18:00

iansan5653 marked this pull request as ready for review July 18, 2024 18:00

iansan5653 requested a review from a team as a code owner July 18, 2024 18:00

iansan5653 requested a review from keithamus July 18, 2024 18:00

Format

46c0135

keithamus reviewed Jul 18, 2024

View reviewed changes

iansan5653 added 3 commits July 18, 2024 18:29

Replace space with "space"

9b131f3

Update exports snapshot

e0345aa

derp, fix my dumb mistakes

d7eb853

This comment was marked as resolved.

Sign in to view

Try canUseDOM instead of window !== undefined

daea8e6

iansan5653 temporarily deployed to github-pages July 18, 2024 19:35 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4750 July 18, 2024 19:36 Inactive

iansan5653 mentioned this pull request Jul 19, 2024

Proposal: Keybinding hints in tooltips / icon buttons #4759

Closed

Merge branch 'main' into add-keybinding-hint

41e1a66

iansan5653 temporarily deployed to github-pages July 22, 2024 17:39 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4750 July 22, 2024 17:40 Inactive

Move to draft status

035d5e9

iansan5653 commented Jul 25, 2024

View reviewed changes

.vscode/settings.json Show resolved Hide resolved

iansan5653 added 3 commits August 6, 2024 13:12

Merge branch 'main' into add-keybinding-hint

626d29b

Update import in docs

6d0aedf

Merge branch 'add-keybinding-hint' of https://github.com/primer/react …

f96742a

…into add-keybinding-hint

iansan5653 requested a review from broccolinisoup August 6, 2024 17:18

iansan5653 temporarily deployed to github-pages August 6, 2024 17:21 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4750 August 6, 2024 17:22 Inactive

iansan5653 added 2 commits August 9, 2024 14:45

Form & update tests

7ca0d77

Merge branch 'main' into add-keybinding-hint

058d9fb

iansan5653 temporarily deployed to github-pages August 9, 2024 17:29 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4750 August 9, 2024 17:29 Inactive

Update snapshots, again

c03d046

iansan5653 temporarily deployed to github-pages August 12, 2024 15:15 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4750 August 12, 2024 15:16 Inactive

mperrotti reviewed Aug 13, 2024

View reviewed changes

Move stories to Drafts

c378c1e

iansan5653 deployed to github-pages August 13, 2024 14:27 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-4750 August 13, 2024 14:28 View deployment

broccolinisoup reviewed Aug 14, 2024

View reviewed changes

siddharthkp reviewed Aug 16, 2024

View reviewed changes

broccolinisoup approved these changes Aug 16, 2024

View reviewed changes

iansan5653 added this pull request to the merge queue Aug 16, 2024

Merged via the queue into main with commit 414c140 Aug 16, 2024
30 checks passed

iansan5653 deleted the add-keybinding-hint branch August 16, 2024 14:26

primer bot mentioned this pull request Aug 16, 2024

Release tracking (37.0.0-rc.2) #4850

Merged

This was referenced Sep 3, 2024

ActionList.Leading/TrailingVisual is invisible to screen readers #4908

Open

TextInput leading/trailingVisual is not semantically associated with the input #4909

Closed

primer bot mentioned this pull request Oct 18, 2024

Release tracking (37.0.0) #5143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `KeybindingHint` component #4750

Add `KeybindingHint` component #4750

iansan5653 commented Jul 18, 2024 •

edited

Loading

changeset-bot bot commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

iansan5653 Jul 18, 2024

keithamus left a comment

keithamus Jul 18, 2024

iansan5653 Jul 18, 2024

broccolinisoup Aug 2, 2024

iansan5653 Aug 6, 2024 •

edited

Loading

broccolinisoup Aug 14, 2024

This comment was marked as resolved.

mperrotti Aug 13, 2024

iansan5653 Aug 13, 2024

broccolinisoup left a comment

broccolinisoup Aug 14, 2024

broccolinisoup Aug 14, 2024

broccolinisoup Aug 14, 2024

broccolinisoup Aug 14, 2024

siddharthkp Aug 16, 2024

siddharthkp Aug 16, 2024 •

edited

Loading

broccolinisoup left a comment

iansan5653 commented Aug 16, 2024

		<VisuallyHidden>{accessibleKeyName(name, isMacOS)}</VisuallyHidden>
		<span aria-hidden>{format === 'condensed' ? condensedKeyName(name, isMacOS) : fullKeyName(name, isMacOS)}</span>

Add KeybindingHint component #4750

Add KeybindingHint component #4750

Conversation

iansan5653 commented Jul 18, 2024 • edited Loading

Changelog

New

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Jul 18, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Jul 18, 2024 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

keithamus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iansan5653 Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broccolinisoup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthkp Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

broccolinisoup left a comment

Choose a reason for hiding this comment

iansan5653 commented Aug 16, 2024

Add `KeybindingHint` component #4750

Add `KeybindingHint` component #4750

iansan5653 commented Jul 18, 2024 •

edited

Loading

changeset-bot bot commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

iansan5653 Aug 6, 2024 •

edited

Loading

siddharthkp Aug 16, 2024 •

edited

Loading