Keyboard Shortcut Pattern #5886
Conversation
🦋 Changeset detectedLatest commit: 7b953e0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
.changeset/rude-heads-report.md
Outdated
| "@salt-ds/lab": minor | ||
| --- | ||
|
|
||
| KS pattern - Added KS pattern and KeyboardKey component. |
There was a problem hiding this comment.
Treat changelogs as documentation, does every Salt user know what KS is ?
So I would say Keyboard Shortcuts rather than KS and describe what the pattern is, so when I read the changelog, I would say, "I know what that is".
| title: "Patterns/Keyboard Shortcuts", | ||
| } as Meta; | ||
|
|
||
| // Keyboard shortcut data type |
There was a problem hiding this comment.
NIT this comment adds no value, as the type KeyboardShortcut offers the same description, so we could remove.
| // Keyboard shortcut data type | ||
| type KeyboardShortcut = { | ||
| label: string; | ||
| shortcut: string[][]; |
There was a problem hiding this comment.
adding a comment here may make sense as it's no immediate obvious why it's an array of arrays.
| label: "Open command palette", | ||
| shortcut: [["meta", "option", "p"]], | ||
| connector: "+", | ||
| action: () => alert("Open command palette triggered!"), |
There was a problem hiding this comment.
NIT remove exclamation marks
| import { KeyboardKey, Table, TBody, TD, TH, THead, TR } from "@salt-ds/lab"; | ||
| import type { Meta } from "@storybook/react-vite"; | ||
| import React, { type ChangeEvent, type SyntheticEvent, useState } from "react"; | ||
| import { HotkeysProvider, useHotkeys } from "react-hotkeys-hook"; |
There was a problem hiding this comment.
are there any types we can use from there ?
There was a problem hiding this comment.
I found two types. Not many
| }, | ||
| ]; | ||
|
|
||
| // Utility: highlight search match |
There was a problem hiding this comment.
This function will be used in code, where this comment does not exist.
So when I see a comment on a function, I often ask myself, can I improve the naming and remove the comment.
In this case, I am not sure we need a comment, the naming makes sense.
|
|
||
| return ( | ||
| <kbd ref={ref} className={clsx(withBaseName(), className, {})} {...props}> | ||
| <Text>{children}</Text> |
There was a problem hiding this comment.
We don't need to use here we can just put the styling on the root element via CSS
There was a problem hiding this comment.
dont use the text element and add the style to the kbd instead
| > | ||
| {comboArr.map((keyName, idx) => ( | ||
| <React.Fragment key={keyName + idx}> | ||
| <KeyboardKey aria-label={displayKeyName(keyName)}> |
There was a problem hiding this comment.
is aria-label needed if the text node is the same ?
| <FlexLayout align="center" gap={1}> | ||
| <Text>hit </Text> | ||
| <FlexLayout align="center" gap={0}> | ||
| <KeyboardKey>ctrl</KeyboardKey>+<KeyboardKey>shift</KeyboardKey>+ |
There was a problem hiding this comment.
Ctrl isn't the right key, you've set it to meta
| // Register all hotkeys when enabled | ||
| const RegisterShortcuts: React.FC<{ enabled: boolean }> = ({ enabled }) => { | ||
| keyboardShortcuts.forEach((shortcut) => { | ||
| shortcut.shortcut.forEach((comboArr) => { |
There was a problem hiding this comment.
hook violation
We want to have examples as simple copy and paste or to be read and understood, without layers of abstractions that hide what is happening.
I would accept some duplication in stories if it creates better clarity.
| Actions | ||
| </Text> | ||
| <StackLayout gap={filteredShortcuts.length ? 3 : 0.75}> | ||
| <ComboBox |
There was a problem hiding this comment.
Should this just be an input?
| Actions | ||
| </Text> | ||
| <StackLayout gap={filteredShortcuts.length ? 3 : 0.75}> | ||
| <ComboBox |
There was a problem hiding this comment.
This control doesn't have an accessible label
| s.label.toLowerCase().includes(filter.trim().toLowerCase()), | ||
| ); | ||
|
|
||
| // Handlers |
packages/lab/src/kbd/Kbd.css
Outdated
| @@ -0,0 +1,16 @@ | |||
| .saltKeyboardKey { | |||
| display: flex; | |||
There was a problem hiding this comment.
This should probably be inline-flex
|
|
||
| import keyboardKeyCss from "./KeyboardKey.css"; | ||
|
|
||
| export interface KeyboardKeyProps extends HTMLAttributes<HTMLElement> {} |
There was a problem hiding this comment.
| export interface KeyboardKeyProps extends HTMLAttributes<HTMLElement> {} | |
| export interface KeyboardKeyProps extends ComponentPropsWithoutRef<"kbd"> {} |
| {/* Register all shortcuts, only when enabled */} | ||
| <RegisterShortcuts enabled={shortcutsEnabled} /> | ||
| <StackLayout gap={1}> | ||
| <Button data-testid="dialog-button" onClick={handleDialogOpen}> |
There was a problem hiding this comment.
Should this toggle open/close?
There was a problem hiding this comment.
But the dialog button is not visible when the dialog is open. Visually not able to toggle it.
| }); | ||
|
|
||
| return ( | ||
| <kbd ref={ref} className={clsx(withBaseName(), className, {})} {...props}> |
There was a problem hiding this comment.
| <kbd ref={ref} className={clsx(withBaseName(), className, {})} {...props}> | |
| <kbd ref={ref} className={clsx(withBaseName(), className)} {...props}> |
| <Text className="keyboardShortcuts-actions-title" styleAs="h3"> | ||
| Actions | ||
| </Text> | ||
| <StackLayout gap={filteredShortcuts.length ? 3 : 0.75}> |
|
|
||
| const withBaseName = makePrefixer("saltKeyboardKey"); | ||
|
|
||
| export const KeyboardKey = forwardRef<HTMLDivElement, KeyboardKeyProps>( |
There was a problem hiding this comment.
This the see called KBD, like we name Table elements ?
There was a problem hiding this comment.
I have renamed it to Kbd following the Pascal case in React. Let me know if it make sense to be KBD.
|
|
||
| return ( | ||
| <HotkeysProvider> | ||
| {/* Register all shortcuts, only when enabled */} |
There was a problem hiding this comment.
We should align to the docs..
Disabled shortcuts are done this way
https://react-hotkeys-hook.vercel.app/docs/documentation/useHotkeys/disable-hotkeys
There was a problem hiding this comment.
Enter a non-match into the filter and the layout collapses.
There was a problem hiding this comment.
@honey-chang should I add some min-height to maintain the height of the dialog when search has no match?
| if (key === "meta") return "ctrl"; | ||
| if (key === "option") return "option"; | ||
| if (key === "shift") return "shift"; | ||
| return key; |
There was a problem hiding this comment.
Maybe
return function displayKeyName(key: string): string {
const isMac = navigator.platform.toUpperCase().includes('MAC');
const keyMap: Record<string, string> = {
meta: isMac ? "⌘" : "ctrl",
option: isMac ? "⌥" : "alt",
shift: isMac ? "⇧" : "shift",
};
return keyMap[key] ?? key;
}
| Keyboard shortcuts panel | ||
| </Button> | ||
| <FlexLayout align="center" gap={1} wrap> | ||
| <Text>hit </Text> |
There was a problem hiding this comment.
Could it just be press ?
| Term | Usage | Why |
|---|---|---|
| Press | ✅ Standard | Official term in WCAG, ARIA, and UI guidelines |
| Hit | ❌ Informal | Sounds aggressive, not professional |
| @@ -0,0 +1,220 @@ | |||
| --- | |||
| title: Keyboard Shortcuts | |||
| description: "Keyboard shortcuts allow users to execute actions, using a keyboard, that are usually performed with a mouse or track pad. They increase the efficiency and productivity of users who primarily rely on their keyboard when interacting with applications." | |||
| ] | ||
| components: ["Avatar", "Button", "Flow layout", "Icon", "Stack layout", "Text", "Tooltip"] | ||
| --- | ||
| The keyboard shortcuts displays a comprehensive, searchable list of all available shortcuts, helping users quickly learn and reference key combinations to navigate and perform actions efficiently. |
There was a problem hiding this comment.
all available has some redundancy, can just be available.
|
|
||
| Use this pattern to: | ||
| - Increase task efficiency by enabling users to perform actions quickly. | ||
| - Provide a true alternative to mouse or track pad navigation, supporting users who prefer or require keyboard input. |
There was a problem hiding this comment.
The spell check in site was throwing error for trackpad as a single work. let me check again.
| - Increase task efficiency by enabling users to perform actions quickly. | ||
| - Provide a true alternative to mouse or track pad navigation, supporting users who prefer or require keyboard input. | ||
| - Clearly communicate available shortcuts to help users discover and learn efficient workflows. | ||
| - Allow users to search for and execute actions directly from the keyboard, improving accessibility and user experience. |
There was a problem hiding this comment.
this does not sound right... you can search and you can execute but you do not execute an action from the search. Maybe search needs a clearer description ?
There was a problem hiding this comment.
@honey-chang can you help with the same? I search, we only search for a shortcut. the execution happens separately.
|
|
||
| ### Anatomy | ||
|
|
||
| 1. **Keyboard shortcuts switch**: A switch that allows users to turn the keyboard shortcuts list on or off. |
There was a problem hiding this comment.
is it on or off or is it enable/disable... In our language we have disabled components, not on or off components.
There was a problem hiding this comment.
@honey-chang , should i change it list enabled/ disabled?
| 1. **Keyboard shortcuts switch**: A switch that allows users to turn the keyboard shortcuts list on or off. | ||
| 2. **Filter**: This allows users to quickly search and locate specific shortcuts by typing keywords. | ||
| 3. **Action name**: Actions represent user-initiated commands—such as Save or Delete—that correspond to defined shortcuts. Each action should be clearly named to reflect the UI command executed by the shortcut. | ||
| 4. **Description**: For actions that are unique to your application or require additional context, a brief description can be provided to help users understand their purpose. |
There was a problem hiding this comment.
I noticed we do not match on this, in the search, is that not recommended ?
There was a problem hiding this comment.
Hi Mark, currently the search matches only the shortcut name (like we have in UI toolkit). Let me know if I should add something here.
~I am unsure what the comment means.
| 2. **Filter**: This allows users to quickly search and locate specific shortcuts by typing keywords. | ||
| 3. **Action name**: Actions represent user-initiated commands—such as Save or Delete—that correspond to defined shortcuts. Each action should be clearly named to reflect the UI command executed by the shortcut. | ||
| 4. **Description**: For actions that are unique to your application or require additional context, a brief description can be provided to help users understand their purpose. | ||
| 3. **Key**: Each keyboard shortcut consists of two types of keys—modifier keys and character keys—which work together to trigger the action. |
There was a problem hiding this comment.
sounds wordy
Option 1: Simpler
Key: Each keyboard shortcut combines modifier keys (like Ctrl or Shift) with character keys to trigger an action.
Option 2: More Concise
Key: Keyboard shortcuts combine modifier keys and character keys to trigger actions.
Option 3: Active Voice
Key: Press modifier keys (such as Ctrl, Shift, or Alt) together with character keys to trigger shortcuts.
Option 4: With Examples
Key: Each shortcut combines modifier keys (Ctrl, Shift, Alt, Cmd) with character keys (letters, numbers, symbols) to perform an action.
Option 5: Most Natural
Key: Keyboard shortcuts use a combination of modifier keys and character keys to perform actions.
There was a problem hiding this comment.
@honey-chang , should i keep option 4 with examples sounds more informative and has a flow too. thoughts?
| alt="Keyboard shortcuts with all available shortcuts." | ||
| /> | ||
|
|
||
| 2. When the user enters text in the filter input, the list updates to show only the matching shortcuts. For more details on filtering behavior, refer to the List Filtering pattern page. |
There was a problem hiding this comment.
should that be a link to List Filtering pattern
PR has below:
1.) new component in Labs for KeyboardKey
2.)Keyboard shortcut stories has below: