Skip to content
Draft
Show file tree
Hide file tree
Changes from 4 commits
Commits
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/rude-heads-report.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@salt-ds/lab": minor
---

KS pattern - Added KS pattern and KeyboardKey component.
Copy link
Contributor

Choose a reason for hiding this comment

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

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".

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
"react": "^18.3.1",
"react-docgen-typescript": "2.4.0",
"react-dom": "^18.3.1",
"react-hotkeys-hook": "^5.1.0",
"react-resizable-panels": "^3.0.0",
"react-router": "^7.6.3",
"storybook": "^9.0.4",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.keyboardShortcuts-description {
padding: var(--salt-spacing-25) 0;
}

.keyboardShortcuts-actions-title {
font-weight: var(--salt-text-heading-fontWeight);
}
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Original file line number Diff line number Diff line change
@@ -0,0 +1,281 @@
import {
Button,
ComboBox,
Dialog,
DialogContent,
FlexLayout,
Label,
StackLayout,
Switch,
Text,
} from "@salt-ds/core";
import { FilterIcon } from "@salt-ds/icons";
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";
Copy link
Contributor

Choose a reason for hiding this comment

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

are there any types we can use from there ?

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 found two types. Not many

import "./keyboard-shortcuts.stories.css";

export default {
title: "Patterns/Keyboard Shortcuts",
} as Meta;

// Keyboard shortcut data type
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT this comment adds no value, as the type KeyboardShortcut offers the same description, so we could remove.

type KeyboardShortcut = {
label: string;
shortcut: string[][];
Copy link
Contributor

Choose a reason for hiding this comment

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

adding a comment here may make sense as it's no immediate obvious why it's an array of arrays.

connector: string;
action: () => void;
description?: string;
};

const keyboardShortcuts: KeyboardShortcut[] = [
{
label: "Open command palette",
shortcut: [["meta", "option", "p"]],
connector: "+",
action: () => alert("Open command palette triggered!"),
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT remove exclamation marks

},
{
label: "Next",
shortcut: [["meta", "shift", "e"]],
connector: "+",
action: () => alert("Next triggered!"),
},
{
label: "Previous",
shortcut: [["meta", "e"]],
connector: "+",
action: () => alert("Previous triggered!"),
},
{
label: "Duplicate ticket",
description: "Make a copy of your ticket",
shortcut: [["meta", "d"]],
connector: "+",
action: () => alert("Duplicate ticket triggered!"),
},
{
label: "Set direction to buy",
shortcut: [["meta", "b"]],
connector: "+",
action: () => alert("Set direction to buy triggered!"),
},
{
label: "Set direction to sell",
shortcut: [["meta", "s"]],
connector: "+",
action: () => alert("Set direction to sell triggered!"),
},
{
label: "Bottom of list",
shortcut: [["meta", "end"]],
connector: "+",
action: () => alert("Bottom of list triggered!"),
},
{
label: "Top of list",
shortcut: [["meta", "home"]],
connector: "+",
action: () => alert("Top of list triggered!"),
},
{
label: "Test",
shortcut: [
["meta", "u"], // Cmd+T
["meta", "y"], // Cmd+Y
],
connector: "+",
action: () => alert("Test shortcut triggered!"),
description: "Trigger test action with Cmd+U or Cmd+Y",
},
];

// Utility: highlight search match
Copy link
Contributor

@mark-tate mark-tate Dec 8, 2025

Choose a reason for hiding this comment

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

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.

function highlightMatch(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
),
);
}

function displayKeyName(key: string) {
if (key === "meta") return "ctrl";
return key;
}

// Table row for a shortcut
const ShortcutRow: React.FC<{ shortcut: KeyboardShortcut; filter: string }> = ({
shortcut,
filter,
}) => (
<TR>
<TD>
<StackLayout gap={0.5}>
<Text>{highlightMatch(shortcut.label, filter)}</Text>
{shortcut.description && (
<Label color="secondary">{shortcut.description}</Label>
)}
</StackLayout>
</TD>
<TD>
<StackLayout gap={0.5}>
{shortcut.shortcut.map((comboArr, comboIdx) => (
<FlexLayout
align="center"
gap={0.5}
key={comboArr.join("-") + comboIdx}
>
{comboArr.map((keyName, idx) => (
<React.Fragment key={keyName + idx}>
<KeyboardKey aria-label={displayKeyName(keyName)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

is aria-label needed if the text node is the same ?

{displayKeyName(keyName)}
</KeyboardKey>
{idx < comboArr.length - 1 && <Text>{shortcut.connector}</Text>}
</React.Fragment>
))}
</FlexLayout>
))}
</StackLayout>
</TD>
</TR>
);

// Register all hotkeys when enabled
const RegisterShortcuts: React.FC<{ enabled: boolean }> = ({ enabled }) => {
keyboardShortcuts.forEach((shortcut) => {
shortcut.shortcut.forEach((comboArr) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const combo = comboArr.join("+");
useHotkeys(

Check failure on line 156 in packages/core/stories/patterns/keyboard-shortcuts/keyboard-shortcuts.stories.tsx

View workflow job for this annotation

GitHub Actions / lint

lint/correctness/useHookAtTopLevel

This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
combo,
(event) => {
event.preventDefault();
if (enabled) shortcut.action();
},
{ enabled },
);
});
});
return null;
};

const KeyboardShortcuts: React.FC = () => {
const [open, setOpen] = useState(false);
const [shortcutsEnabled, setShortcutsEnabled] = useState(true);
const [filter, setFilter] = useState("");

useHotkeys(
"meta+shift+k", // To open the keyboard shortcut key panel
(event) => {
event.preventDefault();
setOpen(true);
},
{ enabled: shortcutsEnabled },
);

const filteredShortcuts = keyboardShortcuts.filter((s) =>
s.label.toLowerCase().includes(filter.trim().toLowerCase()),
);

// Handlers
Copy link
Contributor

Choose a reason for hiding this comment

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

lo-value comment

const handleDialogOpen = () => setOpen(true);
const handleDialogChange = (value: boolean) => setOpen(value);
const handleSwitchChange = (event: ChangeEvent<HTMLInputElement>) =>
setShortcutsEnabled(event.target.checked);
const handleFilterChange = (event: ChangeEvent<HTMLInputElement>) =>
setFilter(event.target.value);
const handleFilterSelectionChange = (
_: SyntheticEvent,
newSelected: string[],
) => setFilter(newSelected.length === 1 ? newSelected[0] : "");

return (
<HotkeysProvider>
{/* Register all shortcuts, only when enabled */}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should align to the docs..
Disabled shortcuts are done this way
https://react-hotkeys-hook.vercel.app/docs/documentation/useHotkeys/disable-hotkeys

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

<RegisterShortcuts enabled={shortcutsEnabled} />
<StackLayout gap={1}>
<Button data-testid="dialog-button" onClick={handleDialogOpen}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this toggle open/close?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it just be press ?

Term Usage Why
Press ✅ Standard Official term in WCAG, ARIA, and UI guidelines
Hit ❌ Informal Sounds aggressive, not professional

<FlexLayout align="center" gap={0}>
<KeyboardKey>ctrl</KeyboardKey>+<KeyboardKey>shift</KeyboardKey>+
Copy link
Contributor

Choose a reason for hiding this comment

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

Ctrl isn't the right key, you've set it to meta

<KeyboardKey>K</KeyboardKey>
</FlexLayout>
<Text>to open the keyboard shortcuts panel </Text>
</FlexLayout>
</StackLayout>
<Dialog
open={open}
onOpenChange={handleDialogChange}
id="keyboard-shortcuts-dialog"
size="medium"
>
<DialogContent>
<StackLayout gap={3}>
<FlexLayout gap={1}>
<Switch
checked={shortcutsEnabled}
onChange={handleSwitchChange}
/>
<FlexLayout className="keyboardShortcuts-description">
<Text>Turn on keyboard shortcuts</Text>
</FlexLayout>
</FlexLayout>
Copy link
Contributor

@jake-costa jake-costa Dec 16, 2025

Choose a reason for hiding this comment

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

Suggested change
<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?

Copy link
Contributor Author

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.

{shortcutsEnabled && (
<StackLayout gap={1}>
<Text className="keyboardShortcuts-actions-title" styleAs="h3">
Actions
</Text>
<StackLayout gap={filteredShortcuts.length ? 3 : 0.75}>
Copy link
Contributor

Choose a reason for hiding this comment

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

gap = 0.75?

<ComboBox
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be an input?

Copy link
Contributor

Choose a reason for hiding this comment

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

This control doesn't have an accessible label

onChange={handleFilterChange}
onSelectionChange={handleFilterSelectionChange}
value={filter}
bordered
variant="secondary"
placeholder="Filter actions"
startAdornment={<FilterIcon color="secondary" />}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
startAdornment={<FilterIcon color="secondary" />}
startAdornment={<FilterIcon color="secondary" aria-hidden="true" />}

Should add aria-hidden to properly hide the SVG from screen readers.

/>
{filteredShortcuts.length ? (
<StackLayout style={{ overflow: "auto" }}>
<Table>
<THead>
<TR>
<TH>Action</TH>
<TH>Key combination</TH>
</TR>
</THead>
<TBody>
{filteredShortcuts.map((shortcut, idx) => (
<ShortcutRow
key={shortcut.label + idx}
shortcut={shortcut}
filter={filter}
/>
))}
</TBody>
</Table>
</StackLayout>
) : (
<Text color="secondary">No actions found</Text>
)}
</StackLayout>
</StackLayout>
)}
</StackLayout>
</DialogContent>
</Dialog>
</HotkeysProvider>
);
};

export const WithKeyboardShortcutExample = KeyboardShortcuts.bind({});
1 change: 1 addition & 0 deletions packages/lab/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export {
type InputLegacyProps as InputProps,
StaticInputAdornment,
} from "./input-legacy";
export * from "./keyboard-key";
export * from "./layer-layout";
export * from "./list";
export type {
Expand Down
16 changes: 16 additions & 0 deletions packages/lab/src/keyboard-key/KeyboardKey.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
.saltKeyboardKey {
display: flex;
--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);
min-height: calc(var(--salt-size-base) - var(--salt-spacing-100));
align-items: center;
padding: 0 var(--salt-spacing-50);
}

.saltKeyboardKey .saltText {
width: fit-content;
}
31 changes: 31 additions & 0 deletions packages/lab/src/keyboard-key/KeyboardKey.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { makePrefixer, Text } from "@salt-ds/core";
import { useComponentCssInjection } from "@salt-ds/styles";
import { useWindow } from "@salt-ds/window";
import { clsx } from "clsx";
import { forwardRef, type HTMLAttributes, useEffect, useState } from "react";

Check failure on line 5 in packages/lab/src/keyboard-key/KeyboardKey.tsx

View workflow job for this annotation

GitHub Actions / lint

lint/correctness/noUnusedImports

Several of these imports are unused.
import { useIsViewportLargerThanBreakpoint } from "../utils";

Check failure on line 6 in packages/lab/src/keyboard-key/KeyboardKey.tsx

View workflow job for this annotation

GitHub Actions / lint

lint/correctness/noUnusedImports

This import is unused.

import keyboardKeyCss from "./KeyboardKey.css";

export interface KeyboardKeyProps extends HTMLAttributes<HTMLElement> {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export interface KeyboardKeyProps extends HTMLAttributes<HTMLElement> {}
export interface KeyboardKeyProps extends ComponentPropsWithoutRef<"kbd"> {}


const withBaseName = makePrefixer("saltKeyboardKey");

export const KeyboardKey = forwardRef<HTMLDivElement, KeyboardKeyProps>(
Copy link
Contributor

Choose a reason for hiding this comment

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

This the see called KBD, like we name Table elements ?

Copy link
Contributor Author

@ThusharaJ07 ThusharaJ07 Dec 16, 2025

Choose a reason for hiding this comment

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

I have renamed it to Kbd following the Pascal case in React. Let me know if it make sense to be KBD.

function KeyboardKey(props, ref) {
const { children, className, ...rest } = props;

const targetWindow = useWindow();
useComponentCssInjection({
testId: "salt-keyboard-key",
css: keyboardKeyCss,
window: targetWindow,
});

return (
<kbd ref={ref} className={clsx(withBaseName(), className, {})} {...props}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<kbd ref={ref} className={clsx(withBaseName(), className, {})} {...props}>
<kbd ref={ref} className={clsx(withBaseName(), className)} {...props}>

<Text>{children}</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to use here we can just put the styling on the root element via CSS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dont use the text element and add the style to the kbd instead

</kbd>
);
},
);
1 change: 1 addition & 0 deletions packages/lab/src/keyboard-key/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./KeyboardKey";
11 changes: 11 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3929,6 +3929,7 @@ __metadata:
react: "npm:^18.3.1"
react-docgen-typescript: "npm:2.4.0"
react-dom: "npm:^18.3.1"
react-hotkeys-hook: "npm:^5.1.0"
react-resizable-panels: "npm:^3.0.0"
react-router: "npm:^7.6.3"
rollup: "npm:^4.24.2"
Expand Down Expand Up @@ -14706,6 +14707,16 @@ __metadata:
languageName: node
linkType: hard

"react-hotkeys-hook@npm:^5.1.0":
version: 5.1.0
resolution: "react-hotkeys-hook@npm:5.1.0"
peerDependencies:
react: ">=16.8.0"
react-dom: ">=16.8.0"
checksum: 10/e912c0178fbfb04b841007aa3fc50cb326082346f860d3b92a22ff71516dfd487c6f60c0474d9609b982ad15e6b4741c35ca2b38adfeb133789d43ff6b0029c6
languageName: node
linkType: hard

"react-immutable-proptypes@npm:2.2.0":
version: 2.2.0
resolution: "react-immutable-proptypes@npm:2.2.0"
Expand Down
Loading