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

Add KeybindingHint component #4750

Merged
merged 26 commits into from
Aug 16, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9104157
Add `KeybindingHint` component
iansan5653 Jul 18, 2024
66dc77e
Split file and refactor a bit
iansan5653 Jul 18, 2024
04904cd
Split components out into individual files
iansan5653 Jul 18, 2024
cec4e50
Update comments
iansan5653 Jul 18, 2024
8a37a6f
Create `useIsMacOS` hook for SSR support
iansan5653 Jul 18, 2024
b0e1e7a
Add changelog
iansan5653 Jul 18, 2024
46c0135
Format
iansan5653 Jul 18, 2024
9b131f3
Replace space with "space"
iansan5653 Jul 18, 2024
e0345aa
Update exports snapshot
iansan5653 Jul 18, 2024
d7eb853
derp, fix my dumb mistakes
iansan5653 Jul 18, 2024
daea8e6
Try `canUseDOM` instead of `window !== undefined`
iansan5653 Jul 18, 2024
41e1a66
Merge branch 'main' into add-keybinding-hint
iansan5653 Jul 22, 2024
035d5e9
Move to draft status
iansan5653 Jul 25, 2024
cbefb0f
Merge branch 'main' into add-keybinding-hint
iansan5653 Jul 26, 2024
69aa696
Move export to drafts
iansan5653 Aug 6, 2024
253b9b3
Separate out `features` stories and add `onEmphasis` story
iansan5653 Aug 6, 2024
acbd25b
Add examples stories
iansan5653 Aug 6, 2024
0562fa2
Tweak styles
iansan5653 Aug 6, 2024
03b6491
Remove comma between chords
iansan5653 Aug 6, 2024
626d29b
Merge branch 'main' into add-keybinding-hint
iansan5653 Aug 6, 2024
6d0aedf
Update import in docs
iansan5653 Aug 6, 2024
f96742a
Merge branch 'add-keybinding-hint' of https://github.com/primer/react…
iansan5653 Aug 6, 2024
7ca0d77
Form & update tests
iansan5653 Aug 9, 2024
058d9fb
Merge branch 'main' into add-keybinding-hint
iansan5653 Aug 9, 2024
c03d046
Update snapshots, again
iansan5653 Aug 12, 2024
c378c1e
Move stories to Drafts
iansan5653 Aug 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/short-boats-cover.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

Add `KeybindingHint` component for indicating an available keyboard shortcut
157 changes: 157 additions & 0 deletions docs/content/KeybindingHint.mdx
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
---
title: KeybindingHint
componentId: keybinding_hint
status: Alpha
source: https://github.com/primer/react/tree/main/packages/react/src/KeybindingHint
storybook: '/react/storybook?path=/story/components-keybindinghint'
description: Indicates the presence of a keybinding available for an action.
---

import data from '../../packages/react/src/KeybindingHint/KeybindingHint.docs.json'
import {ActionList, KeybindingHint, Button, Text, Box} from '@primer/react'
import {TrashIcon} from '@primer/octicons-react'

Use `KeybindingHint` to make keyboard shortcuts discoverable. Can render visual keybinding hints in condensed (abbreviated) form or expanded form, and provides accessible alternative text for screen reader users.

<Box sx={{border: '1px solid', borderColor: 'border.default', borderRadius: 2, padding: 6, marginBottom: 3}}>
<ActionList sx={{width: 320}}>
<ActionList.Item>
Move down
<ActionList.TrailingVisual>
<KeybindingHint keys="Mod+ArrowDown" />
</ActionList.TrailingVisual>
</ActionList.Item>
<ActionList.Item>
Unsubscribe
<ActionList.TrailingVisual>
<KeybindingHint keys="i j" />
</ActionList.TrailingVisual>
</ActionList.Item>
<ActionList.Item variant="danger">
<ActionList.LeadingVisual>
<TrashIcon />
</ActionList.LeadingVisual>
Delete
<ActionList.TrailingVisual>
<KeybindingHint keys="Mod+Shift+Delete" />
</ActionList.TrailingVisual>
</ActionList.Item>
</ActionList>
</Box>

## Examples

### Single keys

Use the [full names of the keys as returned by `KeyboardEvent.key`](https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_key_values). Key names are case-insensitive.

```javascript live noinline
render(
<>
<KeybindingHint keys="a" /> <br />
<KeybindingHint keys="B" /> <br />
<KeybindingHint keys="ArrowLeft" /> <br />
<KeybindingHint keys="shift" />
</>,
)
```

#### Special key names

Because the `+` and ` `&nbsp;(space) characters are used to build chords and sequences as described below, their names must be spelled out to be used as keys.

```javascript live noinline
render(
<>
<KeybindingHint keys="Plus" /> <br />
<KeybindingHint keys="Space" />
</>,
)
```

### Chords

_Chords_ are multiple keys that are pressed at the same time. Combine keys in a chord with `+`. Keys are automatically sorted into a standardized order so that modifiers come first.

```javascript live noinline
render(
<>
<KeybindingHint keys="Alt+a" /> <br />
<KeybindingHint keys="a+Alt" /> <br />
<KeybindingHint keys="Control+Shift+ArrowUp" /> <br />
<KeybindingHint keys="Meta+Shift+&" />
</>,
)
```

#### Platform-dependent modifier

Typical chords use `Command` on MacOS and `Control` on other devices. To automatically render `Command` or `Control` based on the user's operating system, use the special key name `Mod`.

```javascript live noinline
render(<KeybindingHint keys="Mod+Shift+X" />)
```

### Sequences

_Sequences_ are keys or chords that are pressed one after the other. Combine elements in a sequence with ` `. For example, `a b` means "press a, then press b".

```javascript live noinline
render(
<>
<KeybindingHint keys="a b" /> <br />
<KeybindingHint keys="Mod+g ArrowLeft" />
</>,
)
```

### Full display format

The default `condensed` format should be used on UI elements like buttons, menuitems, and inputs. In long-form text (prose), the `full` variant can be used instead to help the text flow better.

```javascript live noinline
render(
<Text>
Press <KeybindingHint keys="Mod+Enter" format="ful" /> to submit the form.
</Text>,
)
```

### `onEmphasis` variant

When rendering on 'emphasis' colors, use the `onEmphasis` variant.

```javascript live noinline
const CmdEnterHint = () => <KeybindingHint variant="onEmphasis" keys="Mod+Enter" />

render(
<Button variant="primary" trailingVisual={CmdEnterHint}>
Submit
</Button>,
)
```

## Props

<ComponentProps data={data} />

## Status

<ComponentChecklist
items={{
propsDocumented: true,
noUnnecessaryDeps: true,
adaptsToThemes: true,
adaptsToScreenSizes: true,
fullTestCoverage: true,
usedInProduction: true,
usageExamplesDocumented: true,
hasStorybookStories: true,
designReviewed: false,
a11yReviewed: false,
stableApi: false,
addressedApiFeedback: false,
hasDesignGuidelines: false,
hasFigmaComponent: false,
}}
/>
2 changes: 2 additions & 0 deletions docs/src/@primer/gatsby-theme-doctocat/nav.yml
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@
url: /Heading
- title: IconButton
url: /IconButton
- title: KeybindingHint
url: /KeybindingHint
Copy link
Member

@siddharthkp siddharthkp Aug 16, 2024

Choose a reason for hiding this comment

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

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)

- title: Label
url: /Label
- title: LabelGroup
Expand Down
28 changes: 28 additions & 0 deletions packages/react/src/KeybindingHint/KeybindingHint.docs.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"id": "KeybindingHint",
"name": "KeybindingHint",
"status": "alpha",
"a11yReviewed": false,
"stories": [],
"importPath": "@primer/react",
"props": [
{
"name": "keys",
"type": "string",
"description": "The keys involved in this keybinding."
},
{
"name": "format",
"type": "'condensed' | 'full'",
"defaultValue": "'condensed'",
"description": "Control the display format."
},
{
"name": "variant",
"type": "'normal' | 'onEmphasis'",
"defaultValue": "'normal'",
"description": "Set to `onEmphasis` for display on 'emphasis' colors."
}
],
"subcomponents": []
}
28 changes: 28 additions & 0 deletions packages/react/src/KeybindingHint/KeybindingHint.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import type React from 'react'
import type {Meta} from '@storybook/react'
import {KeybindingHint} from './KeybindingHint'

type KeybindingHintProps = React.ComponentProps<typeof KeybindingHint>

const args = {
keys: 'Mod+Shift+K',
} satisfies KeybindingHintProps

const meta = {
Copy link
Member

Choose a reason for hiding this comment

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

Could we preserve this file name for the default story?

Our practise is to have a default story on <component-name.stories.tsx> then add features on <component-name.features.stories.tsx>. If we have any examples we add them on <component-name.example.stories.tsx> - Would you be happy to structure the stories based on this?

It would be great to have a feature story for onEmphasis as well as some example stories such as keybinding component on ActionList and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Pulled these out to features and also added several examples showing interplay with other components.

Copy link
Contributor Author

@iansan5653 iansan5653 Aug 6, 2024

Choose a reason for hiding this comment

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

I don't think we should resolve this in this PR, but I did notice some accessibility issues with the example stories:

  • In ActionList.Item, the TrailingVisual does not become part of the accessible label. This means that shortcut hints rendered in this manner are invisible to screen readers 😞
  • In Button, the trailingVisual is part of the label but with only a space as separation. This causes the button label to be read as, for example, "Submit command enter".
  • In TextInput, the trailingVisual is read to screen readers as text after the input, not associated semantically with the input at all.

I think in all these cases, the ideal solution would be to append the trailing visual to the semantic label with a comma as a separator. For example, "Submit, command enter" or "Search, forward slash". These issues should probably be addressed in the individual components since they affect more than just keyboard shortcuts.

title: 'Components/KeybindingHint',
iansan5653 marked this conversation as resolved.
Show resolved Hide resolved
component: KeybindingHint,
} satisfies Meta<typeof KeybindingHint>

export default meta

export const Condensed = {args}

export const Full = {args: {...args, format: 'full'}}

const sequenceArgs = {
keys: 'Mod+x y z',
} satisfies KeybindingHintProps

export const SequenceCondensed = {args: sequenceArgs}

export const SequenceFull = {args: {...sequenceArgs, format: 'full'}}
49 changes: 49 additions & 0 deletions packages/react/src/KeybindingHint/KeybindingHint.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import React, {type ReactNode} from 'react'
iansan5653 marked this conversation as resolved.
Show resolved Hide resolved
import {memo} from 'react'
import Text from '../Text'

import {Chord} from './components/Chord'
import type {KeybindingHintProps} from './props'
import {accessibleSequenceString} from './components/Sequence'

/** `kbd` element with style resets. */
const Kbd = ({children}: {children: ReactNode}) => (
<Text
as={'kbd' as 'span'}
sx={{
color: 'inherit',
fontFamily: 'inherit',
fontSize: 'inherit',
border: 'none',
background: 'none',
boxShadow: 'none',
p: 0,
lineHeight: 'unset',
position: 'relative',
overflow: 'visible',
verticalAlign: 'baseline',
}}
>
{children}
</Text>
)

/** Indicates the presence of an available keybinding. */
// KeybindingHint is a good candidate for memoizing since props will rarely change
export const KeybindingHint = memo((props: KeybindingHintProps) => (
<Kbd>
<Chord {...props} />
</Kbd>
))
KeybindingHint.displayName = 'KeybindingHint'
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


/**
* AVOID: `KeybindingHint` is nearly always sufficient for providing both visible and accessible keyboard hints, and
* will result in a good screen reader experience when used as the target for `aria-describedby` and `aria-labelledby`.
* However, there may be cases where we need a plain string version, such as when building `aria-label` or
* `aria-description`. In that case, this plain string builder can be used instead.
*
* NOTE that this string should _only_ be used when building `aria-label` or `aria-description` props (never rendered
* visibly) and should nearly always also be paired with a visible hint for sighted users.
*/
export const getAccessibleKeybindingHintString = accessibleSequenceString
70 changes: 70 additions & 0 deletions packages/react/src/KeybindingHint/components/Chord.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import React, {Fragment} from 'react'
import Text from '../../Text'
import type {KeybindingHintProps} from '../props'
import {Key} from './Key'
import {accessibleKeyName} from '../key-names'

/**
* Consistent sort order for modifier keys. There should never be more than one non-modifier
* key in a chord, so we don't need to worry about sorting those - we just put them at
* the end.
*/
const keySortPriorities: Partial<Record<string, number>> = {
control: 1,
meta: 2,
alt: 3,
option: 4,
shift: 5,
function: 6,
}

const keySortPriority = (priority: string) => keySortPriorities[priority] ?? Infinity

const compareLowercaseKeys = (a: string, b: string) => keySortPriority(a) - keySortPriority(b)

/** Split and sort the chord keys in standard order. */
const splitChord = (chord: string) =>
chord
.split('+')
.map(k => k.toLowerCase())
.sort(compareLowercaseKeys)

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@iansan5653 iansan5653 Aug 6, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

display: 'inline-flex',
bg: variant === 'onEmphasis' ? 'transparent' : 'canvas.default',
color: variant === 'onEmphasis' ? 'fg.onEmphasis' : 'fg.muted',
border: '1px solid',
borderColor: 'border.default',
borderRadius: 2,
fontWeight: 'normal',
fontFamily: 'normal',
fontSize: 0,
p: 1,
gap: '0.5ch',
boxShadow: 'none',
verticalAlign: 'baseline',
overflow: 'hidden',
lineHeight: '10px',
}}
>
{splitChord(keys).map((k, i) => (
<Fragment key={i}>
{i > 0 && format === 'full' ? (
<span aria-hidden> + </span> // hiding the plus sign helps screen readers be more concise
) : (
' ' // space is nonvisual due to flex layout but critical for labelling / screen readers
)}

<Key name={k} format={format} />
</Fragment>
))}
</Text>
)

/** Plain string version of `Chord` for use in `aria` string attributes. */
export const accessibleChordString = (chord: string, isMacOS: boolean) =>
splitChord(chord)
.map(key => accessibleKeyName(key, isMacOS))
.join(' ')
22 changes: 22 additions & 0 deletions packages/react/src/KeybindingHint/components/Key.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import React from 'react'
import VisuallyHidden from '../../_VisuallyHidden'
import {accessibleKeyName, condensedKeyName, fullKeyName} from '../key-names'
import type {KeybindingHintFormat} from '../props'
import {useIsMacOS} from '../../hooks/useIsMacOS'

interface KeyProps {
name: string
format: KeybindingHintFormat
}

/** Renders a single key with accessible alternative text. */
export const Key = ({name, format}: KeyProps) => {
const isMacOS = useIsMacOS()

return (
<>
<VisuallyHidden>{accessibleKeyName(name, isMacOS)}</VisuallyHidden>
<span aria-hidden>{format === 'condensed' ? condensedKeyName(name, isMacOS) : fullKeyName(name, isMacOS)}</span>
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

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

I am curious if this needs to be customisable as well depending on where it is used? 🤔

For example on IconButton and Tooltip, we would need to append the accessile name to the nae or the description. I wonder how this visually hidden element will play out here? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have verified that if we are generating the accessible name/description from DOM, this results in the correct output. For example, if you render this inside of a Button element, the VisuallyHidden part is what becomes part of the accessible name while the aria-hidden part is ignored. This is what we want since the VisuallyHidden part contains the readable string version of the key icons.

For when we are appending the shortcut to a plain string used as aria-label or aria-description, we expose a getAccessibleKeybindingHintString that allows us to skip the whole DOM part and just get a usable string, so that case is covered as well.

Copy link
Member

Choose a reason for hiding this comment

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

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

Tooltip is open on a button and it has keyboard shortcuts on the tooltip. The text colour of the keybinding component doesn't create enough contrast on the tooltip background

tagging @mperrotti here for design 👀

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@iansan5653 iansan5653 Aug 15, 2024

Choose a reason for hiding this comment

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

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

I think this should be resolvable with just variant="onEmphasis", which should flip the text color to work here.

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?

Not really, since the macOS logic is now inside a hook - we'd need to make this a hook as well. There are rare cases where we call this outside the react app, so we'd rather not have this as a hook.

🤔 What we could do is introduce a new hook wrapper around this like useAccessibleKeybindingHintString in addition to getAccessibleKeybindingHintString.

Copy link
Member

Choose a reason for hiding this comment

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

No worries, let's keep it as is 👍🏻

</>
)
}
Loading
Loading