-
Notifications
You must be signed in to change notification settings - Fork 114
Keyboard Shortcut Pattern #5886
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
1696f74
7f38259
8ab154b
661a199
9ea1de9
1915e18
7895f54
8d0bb40
c8680cc
7e95d73
b0363e7
d74ac6a
827849b
cc79d09
0be6461
81d4fae
9dd7f41
00a3f03
16a5818
d42742e
6155c80
7b953e0
67e5ad2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "@salt-ds/lab": minor | ||
| --- | ||
|
|
||
| - Added Kbd component in Labs. | ||
| - Added Keyboard shortcut pattern. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| .keyboardShortcuts-description { | ||
| padding: var(--salt-spacing-25) 0; | ||
| } | ||
| .keyboardShortcuts-actions-title { | ||
| font-weight: var(--salt-text-heading-fontWeight); | ||
| } | ||
| table.saltTable td.keyboardShortcuts-td { | ||
| padding: calc(var(--salt-spacing-50)) var(--salt-spacing-100); | ||
| } | ||
| .keyboardShortcuts-dialog .saltTable { | ||
| --table-row-height: calc(var(--salt-size-base) + var(--salt-spacing-100)); | ||
| } | ||
| .keyboardShortcuts-shortcuts { | ||
| padding: var(--salt-spacing-75) 0; | ||
| } | ||
| .keyboardShortcuts-kbd { | ||
| padding: var(--salt-spacing-50) 0; | ||
| } | ||
| .keyboardShortcuts-dialog .saltDialogContent-inner { | ||
| overflow-y: hidden; | ||
| } | ||
| .keyboardShortcuts-dialog .keyboardShortcuts-tableScroll { | ||
| overflow-y: auto; | ||
| max-height: 40vh; | ||
| position: "relative"; | ||
| } | ||
|
|
||
| .keyboardShortcuts-connector { | ||
| display: inline-block; | ||
| vertical-align: middle; | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,325 @@ | ||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||
| Button, | ||||||||||||||||||||||||||||||
| Dialog, | ||||||||||||||||||||||||||||||
| DialogContent, | ||||||||||||||||||||||||||||||
| FlexLayout, | ||||||||||||||||||||||||||||||
| FormFieldHelperText, | ||||||||||||||||||||||||||||||
| Input, | ||||||||||||||||||||||||||||||
| StackLayout, | ||||||||||||||||||||||||||||||
| Switch, | ||||||||||||||||||||||||||||||
| Text, | ||||||||||||||||||||||||||||||
| } from "@salt-ds/core"; | ||||||||||||||||||||||||||||||
| import { FilterIcon } from "@salt-ds/icons"; | ||||||||||||||||||||||||||||||
| import { Kbd, Table, TBody, TD, TH, THead, TR } from "@salt-ds/lab"; | ||||||||||||||||||||||||||||||
| import type { Meta } from "@storybook/react-vite"; | ||||||||||||||||||||||||||||||
| import React, { type ChangeEvent, type FC, useState } from "react"; | ||||||||||||||||||||||||||||||
| import { HotkeysProvider, useHotkeys } from "react-hotkeys-hook"; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| import "./keyboard-shortcuts.stories.css"; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| export default { | ||||||||||||||||||||||||||||||
| title: "Patterns/Keyboard Shortcuts", | ||||||||||||||||||||||||||||||
| } as Meta; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| type Shortcut = { | ||||||||||||||||||||||||||||||
| label: string; | ||||||||||||||||||||||||||||||
| keys: string[]; | ||||||||||||||||||||||||||||||
| description?: string; | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const shortcutList: Shortcut[] = [ | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| label: "Open command palette", | ||||||||||||||||||||||||||||||
| keys: ["meta+option+p"], | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| label: "Next", | ||||||||||||||||||||||||||||||
| keys: ["meta+shift+e"], | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| label: "Previous", | ||||||||||||||||||||||||||||||
| keys: ["meta+e"], | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| label: "Duplicate ticket", | ||||||||||||||||||||||||||||||
| keys: ["meta+d"], | ||||||||||||||||||||||||||||||
| description: "Make a copy of your ticket", | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| label: "Set direction to buy", | ||||||||||||||||||||||||||||||
| keys: ["meta+b"], | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| label: "Set direction to sell", | ||||||||||||||||||||||||||||||
| keys: ["meta+s"], | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| label: "Bottom of list", | ||||||||||||||||||||||||||||||
| keys: ["meta+end"], | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| label: "Top of list", | ||||||||||||||||||||||||||||||
| keys: ["meta+home"], | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| label: "Test", | ||||||||||||||||||||||||||||||
| keys: ["meta+u", "meta+y"], | ||||||||||||||||||||||||||||||
| description: "Trigger test action with Cmd+U or Cmd+Y", | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| function displayKeyName(key: string): string { | ||||||||||||||||||||||||||||||
| // todo, detect the OS for meta and display ctrl and command accordingly. | ||||||||||||||||||||||||||||||
| if (key === "meta") return "ctrl"; | ||||||||||||||||||||||||||||||
| if (key === "option") return "option"; | ||||||||||||||||||||||||||||||
| if (key === "shift") return "shift"; | ||||||||||||||||||||||||||||||
| return key; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| function highlightTextMatch(text: string, query: string): React.ReactNode { | ||||||||||||||||||||||||||||||
| if (!query) return text; | ||||||||||||||||||||||||||||||
| const regex = new RegExp(`(${query})`, "gi"); | ||||||||||||||||||||||||||||||
| return text | ||||||||||||||||||||||||||||||
| .split(regex) | ||||||||||||||||||||||||||||||
| .map((part, i) => | ||||||||||||||||||||||||||||||
| part.toLowerCase() === query.toLowerCase() ? ( | ||||||||||||||||||||||||||||||
| <strong key={i}>{part}</strong> | ||||||||||||||||||||||||||||||
| ) : ( | ||||||||||||||||||||||||||||||
| part | ||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const KeyboardShortcuts: FC = () => { | ||||||||||||||||||||||||||||||
| const [open, setOpen] = useState<boolean>(false); | ||||||||||||||||||||||||||||||
| const [shortcutsEnabled, setShortcutsEnabled] = useState<boolean>(true); | ||||||||||||||||||||||||||||||
| const [filter, setFilter] = useState<string>(""); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| useHotkeys( | ||||||||||||||||||||||||||||||
| "meta+option+p", | ||||||||||||||||||||||||||||||
| (e) => { | ||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||
| alert("Open command palette triggered"); | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { enabled: shortcutsEnabled }, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| useHotkeys( | ||||||||||||||||||||||||||||||
| "meta+shift+e", | ||||||||||||||||||||||||||||||
| (e) => { | ||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||
| alert("Next triggered"); | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { enabled: shortcutsEnabled }, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| useHotkeys( | ||||||||||||||||||||||||||||||
| "meta+e", | ||||||||||||||||||||||||||||||
| (e) => { | ||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||
| alert("Previous triggered"); | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { enabled: shortcutsEnabled }, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| useHotkeys( | ||||||||||||||||||||||||||||||
| "meta+d", | ||||||||||||||||||||||||||||||
| (e) => { | ||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||
| alert("Duplicate ticket triggered"); | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { enabled: shortcutsEnabled }, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| useHotkeys( | ||||||||||||||||||||||||||||||
| "meta+b", | ||||||||||||||||||||||||||||||
| (e) => { | ||||||||||||||||||||||||||||||
| shortcutsEnabled && alert("Set direction to buy triggered"); | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { enabled: shortcutsEnabled }, | ||||||||||||||||||||||||||||||
| [shortcutsEnabled], | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| useHotkeys( | ||||||||||||||||||||||||||||||
| "meta+s", | ||||||||||||||||||||||||||||||
| (e) => { | ||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||
| alert("Set direction to sell triggered"); | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { enabled: shortcutsEnabled }, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| useHotkeys( | ||||||||||||||||||||||||||||||
| "meta+end", | ||||||||||||||||||||||||||||||
| (e) => { | ||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||
| alert("Bottom of list triggered"); | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { enabled: shortcutsEnabled }, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| useHotkeys( | ||||||||||||||||||||||||||||||
| "meta+home", | ||||||||||||||||||||||||||||||
| (e) => { | ||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||
| alert("Top of list triggered"); | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { enabled: shortcutsEnabled }, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| useHotkeys( | ||||||||||||||||||||||||||||||
| "meta+u", | ||||||||||||||||||||||||||||||
| (e) => { | ||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||
| alert("Test shortcut triggered"); | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { enabled: shortcutsEnabled }, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| useHotkeys( | ||||||||||||||||||||||||||||||
| "meta+y", | ||||||||||||||||||||||||||||||
| (e) => { | ||||||||||||||||||||||||||||||
| e.preventDefault(); | ||||||||||||||||||||||||||||||
| alert("Test shortcut triggered"); | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { enabled: shortcutsEnabled }, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| useHotkeys( | ||||||||||||||||||||||||||||||
| "meta+shift+k", | ||||||||||||||||||||||||||||||
| (event) => { | ||||||||||||||||||||||||||||||
| event.preventDefault(); | ||||||||||||||||||||||||||||||
| setFilter(""); | ||||||||||||||||||||||||||||||
| setOpen(true); | ||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { enabled: shortcutsEnabled }, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const filteredShortcuts: Shortcut[] = shortcutList.filter((s) => | ||||||||||||||||||||||||||||||
| s.label.toLowerCase().includes(filter.trim().toLowerCase()), | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const handleDialogOpen = (): void => { | ||||||||||||||||||||||||||||||
| setFilter(""); | ||||||||||||||||||||||||||||||
| setOpen(true); | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| const handleDialogChange = (value: boolean): void => { | ||||||||||||||||||||||||||||||
| setOpen(value); | ||||||||||||||||||||||||||||||
| if (value) setFilter(""); | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| const handleSwitchChange = (event: ChangeEvent<HTMLInputElement>): void => | ||||||||||||||||||||||||||||||
| setShortcutsEnabled(event.target.checked); | ||||||||||||||||||||||||||||||
| const handleFilterChange = (event: ChangeEvent<HTMLInputElement>): void => | ||||||||||||||||||||||||||||||
| setFilter(event.target.value); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||
| <HotkeysProvider> | ||||||||||||||||||||||||||||||
| <StackLayout gap={1}> | ||||||||||||||||||||||||||||||
| <Button data-testid="dialog-button" onClick={handleDialogOpen}> | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this toggle open/close?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But the dialog button is not visible when the dialog is open. Visually not able to toggle it. |
||||||||||||||||||||||||||||||
| Keyboard shortcuts panel | ||||||||||||||||||||||||||||||
| </Button> | ||||||||||||||||||||||||||||||
| <FlexLayout align="center" gap={1}> | ||||||||||||||||||||||||||||||
| <Text>hit </Text> | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| Term | Usage | Why |
|---|---|---|
| Press | ✅ Standard | Official term in WCAG, ARIA, and UI guidelines |
| Hit | ❌ Informal | Sounds aggressive, not professional |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <FlexLayout gap={1}> | |
| <Switch | |
| checked={shortcutsEnabled} | |
| onChange={handleSwitchChange} | |
| /> | |
| <FlexLayout className="keyboardShortcuts-description"> | |
| <Text>Turn on keyboard shortcuts</Text> | |
| </FlexLayout> | |
| </FlexLayout> | |
| <Switch | |
| checked={shortcutsEnabled} | |
| onChange={handleSwitchChange} | |
| label="Turn on keyboard shortcuts" | |
| /> |
Not sure if there was a specific reason we split the label out into a separate block, but I think this would be a bit cleaner + more consistent with the Switch pattern if we use the label prop. Also, doing it this way ensures the Switch has an accessible name. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, i missed the api for switch. Have added the label as a part of switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gap = 0.75?
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| startAdornment={<FilterIcon color="secondary" />} | |
| startAdornment={<FilterIcon color="secondary" aria-hidden="true" />} |
Should add aria-hidden to properly hide the SVG from screen readers.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| aria-label="Filter actions" | |
| inputProps={{ "aria-label": "Filter actions" }} |
I think we should use inputProps here so the aria-label is applied to the actual <input> element (not the outer wrapper <div>). Thoughts?
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <Kbd aria-label={displayKeyName(key)}> | |
| <Kbd> |
aria-label isn't needed since the value matches the text
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| .saltKbd { | ||
| display: inline-flex; | ||
| box-sizing: border-box; | ||
| --saltText-fontFamily: var(--salt-text-code-fontFamily); | ||
| width: fit-content; | ||
| border-radius: var(--salt-palette-corner-weaker); | ||
| border: var(--salt-size-fixed-100) solid var(--salt-container-primary-borderColor); | ||
| background: var(--salt-container-primary-background); | ||
| box-shadow: 0 var(--salt-size-fixed-100) 0 0 var(--salt-container-primary-borderColor); | ||
| height: calc(var(--salt-size-base) - var(--salt-spacing-100)); | ||
| align-items: center; | ||
| padding: 0 var(--salt-spacing-50); | ||
| text-transform: capitalize; | ||
| } | ||
|
|
||
| .saltKbd .saltText { | ||
| width: fit-content; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enter a non-match into the filter and the layout collapses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@honey-chang should I add some min-height to maintain the height of the dialog when search has no match?