-
Notifications
You must be signed in to change notification settings - Fork 10
feat(OnyxShortcut): implement shortcut component #4429
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?
feat(OnyxShortcut): implement shortcut component #4429
Conversation
🦋 Changeset detectedLatest commit: dbd2a2f The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
…Shortcut components with _unstableUseShortcutSequence composable
…5146e chore: update Playwright screenshots
|
This feature introduces fairly uncommon functionality. I’d really appreciate any feedback on the overall concept, API shape, or naming conventions. If you have ideas for simplifying the internal logic, improving composability, or aligning this with other utilities, please feel free to share them. I’d be happy to iterate on this together. |
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.
Very cool contribution, thanks! 🎉 I haven't found the time yet to review the OnyxShortcut component or shortcut composable (I hope to find time at the end of the week) but here is already some smaller feedback for the OnyxKey component :)
I like the overall API / usage of the components so I'm open to it :)
I just think that there is currently a lot of key mapping going on which adds a lot of code and might be harder to maintain in the future 🤔 Maybe we can streamline this a little bit, what do you think? Maybe we might introduce / limit our custom KeyboardKey type as proposed in one of my comments to define which keys we are expecting?
@JoCa96 What are your thoughts on this?
| /** | ||
| * Example: simple shortcut. | ||
| */ | ||
| export const Shortcut = { |
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.
For displaying shortcuts, we intend to use the OnyxShortcut component right? I'd prefer to remove this example then. Maybe we can update the description at the top to mention that the OnyxShortcut should be used for this?
| /** | ||
| * Displays all supported canonical keys from CANONICAL_KEYS. | ||
| * Useful for visual verification of the mapping. | ||
| */ | ||
| export const AllCanonicalKeys = { | ||
| render: (args) => { | ||
| const keys = CANONICAL_KEYS.filter((key) => key !== "unknown"); | ||
|
|
||
| return { | ||
| components: { OnyxKey }, | ||
| setup: () => ({ args, keys }), | ||
| template: ` | ||
| <div style="display: flex; flex-wrap: wrap; gap: 0.5rem;"> | ||
| <OnyxKey | ||
| v-for="key in keys" | ||
| :key="key" | ||
| v-bind="args" | ||
| :keyName="key" | ||
| /> | ||
| </div> | ||
| `, | ||
| }; | ||
| }, | ||
| args: { | ||
| variant: "macOS", | ||
| }, | ||
| } satisfies Story; |
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.
| /** | |
| * Displays all supported canonical keys from CANONICAL_KEYS. | |
| * Useful for visual verification of the mapping. | |
| */ | |
| export const AllCanonicalKeys = { | |
| render: (args) => { | |
| const keys = CANONICAL_KEYS.filter((key) => key !== "unknown"); | |
| return { | |
| components: { OnyxKey }, | |
| setup: () => ({ args, keys }), | |
| template: ` | |
| <div style="display: flex; flex-wrap: wrap; gap: 0.5rem;"> | |
| <OnyxKey | |
| v-for="key in keys" | |
| :key="key" | |
| v-bind="args" | |
| :keyName="key" | |
| /> | |
| </div> | |
| `, | |
| }; | |
| }, | |
| args: { | |
| variant: "macOS", | |
| }, | |
| } satisfies Story; | |
| /** | |
| * Displays all supported canonical keys from CANONICAL_KEYS. | |
| * Useful for visual verification of the mapping. | |
| */ | |
| export const AllCanonicalKeys = createAdvancedStoryExample( | |
| "OnyxKey", | |
| "CanonicalKeysExample", | |
| ) satisfies Story; |
The downside of using render functions in Storybook is that the generated code snippet in the docs does not reflect the actual code written here. To solve this, we created a createAdvancedStorybookExample() utility function. With it, you can create a dedicated .vue file inside e.g. OnyxKey/examples/CanonicalKeysExample.vue and it will then be rendered as story as well as including the correct source code from the vue file as code snippet
| if (props.variant !== "auto") { | ||
| return props.variant; | ||
| } | ||
| return detectOperatingSystem(); |
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.
It would be great if we can also support SSR (server side rendering), e.g. with Nuxt. Since detectOperatingSystem would be executed on server side, the output might not match the actual users OS so a hydration mismatch would exist.
To solve this, we could e.g. only call the method in onBeforeMount / onMounted or change the plain function to be a composable instead which handles this under the hood
| if (props.label) return props.label; | ||
|
|
||
| const key = canonicalKey.value; | ||
| const os = effectiveOs.value; | ||
|
|
||
| if (typeof key === "string" && isCanonicalSpecialKey(key)) { | ||
| const osMap = MAP_CANONICAL_TO_SCREEN_READER_LABEL_BY_OS[os] ?? {}; | ||
| const genericMap = MAP_CANONICAL_TO_SCREEN_READER_LABEL_BY_OS.generic ?? {}; | ||
|
|
||
| return `${osMap[key] ?? genericMap[key] ?? String(props.keyName)} key`; | ||
| } | ||
|
|
||
| return `${String(props.keyName)} key`; |
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.
| if (props.label) return props.label; | |
| const key = canonicalKey.value; | |
| const os = effectiveOs.value; | |
| if (typeof key === "string" && isCanonicalSpecialKey(key)) { | |
| const osMap = MAP_CANONICAL_TO_SCREEN_READER_LABEL_BY_OS[os] ?? {}; | |
| const genericMap = MAP_CANONICAL_TO_SCREEN_READER_LABEL_BY_OS.generic ?? {}; | |
| return `${osMap[key] ?? genericMap[key] ?? String(props.keyName)} key`; | |
| } | |
| return `${String(props.keyName)} key`; | |
| if (props.label) return props.label; | |
| return `${visualLabel.value} key`; |
Can't we reuse the logic from the visual label? :)
Also ideally we use const { t } = injectI18n() to use as translated label
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.
Since we are using <kbd> here, do we even need an dedicated aria label? The browser should already know that its a keyboard shrotcut + its already labeled by its text content (visualLabel)
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.
Since <kbd /> already has the correct semantics for screen readers, and both macOS (VoiceOver) and Windows read the key symbols or text correctly, we don’t need to generate an automatic aria-label.
aria-label should only be used when the user provides a custom label, but we can also remove this completely once we switch to our own key type. I added it only because it was mentioned in the requirements.
https://github.com/SchwarzIT/onyx/issues/2066 icon should be hidden for screenreader, instead key name should be shown (visually hidden)
| <kbd | ||
| v-else | ||
| :class="['onyx-component', 'onyx-key']" | ||
| :data-os="effectiveOs" | ||
| :data-pressed="props.pressed || undefined" | ||
| :aria-label="ariaLabel" | ||
| > | ||
| {{ visualLabel }} | ||
| </kbd> |
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 | |
| v-else | |
| :class="['onyx-component', 'onyx-key']" | |
| :data-os="effectiveOs" | |
| :data-pressed="props.pressed || undefined" | |
| :aria-label="ariaLabel" | |
| > | |
| {{ visualLabel }} | |
| </kbd> | |
| <kbd | |
| v-else | |
| :class="['onyx-component', 'onyx-key', { 'onyx-key--pressed': props.pressed }]" | |
| :aria-label | |
| > | |
| {{ visualLabel }} | |
| </kbd> |
Would prefer to use a CSS class instead of data attributes :)
| .onyx-key-skeleton, | ||
| .onyx-key { | ||
| @include layers.component() { | ||
| --onyx-key-font-family: monospace; | ||
| --onyx-key-background: var(--onyx-color-base-background-tinted); | ||
| --onyx-key-border-color: var(--onyx-color-base-neutral-300); | ||
| --onyx-key-border-radius: var(--onyx-radius-sm); | ||
| --onyx-key-color: var(--onyx-color-text-icons-neutral-medium); | ||
| --onyx-key-background-pressed: var(--onyx-color-base-neutral-200); | ||
| --onyx-key-border-color-pressed: var(--onyx-color-base-neutral-400); | ||
| --onyx-key-size: 1.5rem; | ||
| } | ||
| } | ||
|
|
||
| .onyx-key { | ||
| @include layers.component() { | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| box-sizing: border-box; | ||
| padding: 0 0.5em; | ||
|
|
||
| height: var(--onyx-key-size); | ||
| min-width: var(--onyx-key-size); | ||
|
|
||
| font-family: var(--onyx-key-font-family); | ||
| font-size: 0.9em; | ||
| line-height: 1; | ||
|
|
||
| background-color: var(--onyx-key-background); | ||
| border: 1px solid var(--onyx-key-border-color); | ||
| border-radius: var(--onyx-key-border-radius); | ||
| color: var(--onyx-key-color); | ||
|
|
||
| &[data-pressed="true"] { | ||
| background-color: var(--onyx-key-background-pressed); | ||
| border-color: var(--onyx-key-border-color-pressed); | ||
| } | ||
| } | ||
| } |
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.
| .onyx-key-skeleton, | |
| .onyx-key { | |
| @include layers.component() { | |
| --onyx-key-font-family: monospace; | |
| --onyx-key-background: var(--onyx-color-base-background-tinted); | |
| --onyx-key-border-color: var(--onyx-color-base-neutral-300); | |
| --onyx-key-border-radius: var(--onyx-radius-sm); | |
| --onyx-key-color: var(--onyx-color-text-icons-neutral-medium); | |
| --onyx-key-background-pressed: var(--onyx-color-base-neutral-200); | |
| --onyx-key-border-color-pressed: var(--onyx-color-base-neutral-400); | |
| --onyx-key-size: 1.5rem; | |
| } | |
| } | |
| .onyx-key { | |
| @include layers.component() { | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| box-sizing: border-box; | |
| padding: 0 0.5em; | |
| height: var(--onyx-key-size); | |
| min-width: var(--onyx-key-size); | |
| font-family: var(--onyx-key-font-family); | |
| font-size: 0.9em; | |
| line-height: 1; | |
| background-color: var(--onyx-key-background); | |
| border: 1px solid var(--onyx-key-border-color); | |
| border-radius: var(--onyx-key-border-radius); | |
| color: var(--onyx-key-color); | |
| &[data-pressed="true"] { | |
| background-color: var(--onyx-key-background-pressed); | |
| border-color: var(--onyx-key-border-color-pressed); | |
| } | |
| } | |
| } | |
| .onyx-key-skeleton, | |
| .onyx-key { | |
| @include layers.component() { | |
| --onyx-key-border-radius: var(--onyx-radius-sm); | |
| --onyx-key-size: 1.5rem; | |
| } | |
| } | |
| .onyx-key { | |
| @include layers.component() { | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| padding-inline: var(--onyx-density-xs); | |
| height: var(--onyx-key-size); | |
| min-width: var(--onyx-key-size); | |
| font-family: var(--onyx-font-family); | |
| font-size: var(--onyx-font-size-md); | |
| line-height: var(--onyx-font-line-height-md); | |
| background-color: var(--onyx-color-base-background-tinted); | |
| border: var(--onyx-1px-in-rem) solid var(--onyx-color-base-neutral-300); | |
| border-radius: var(--onyx-key-border-radius); | |
| color: var(--onyx-color-text-icons-neutral-medium); | |
| &[data-pressed="true"] { | |
| background-color: var(--onyx-color-base-neutral-200); | |
| border-color: var(--onyx-color-base-neutral-400); | |
| } | |
| } | |
| } |
Some style adjustments to match whats defined in Figma :) (Basically the font family and font size changed). Also I'd prefer to remove CSS variables that are only used once to keep it simple, what do you think?
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.
I'm not sure about the code styling, but if you think the absence of specific CSS variables that are used once is better.
I'm also for it :)
| /** | ||
| * The key name (can be canonical or any custom string). | ||
| */ | ||
| keyName: TKeyName; |
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.
Just an idea, let me know what you think:
For a great developer experience, it might be nice to type the keyName as a union type that includes all characters so you get e.g. autocompletion / autosuggestion in the IDE and we can unify it a little bit to e.g. not allow "Space", "space", " " etc. for the same key.
it would mean maintainance effort on our side but it might be worth the DX. What do you think?
Incomplete example code:
export type NumericKey = 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 0;
export type AlphabeticKey =
| "A"
| "B"
| "C"
| "D"
| "E"
| "F"
| "G"
| "H"
| "I"
| "J"
| "K"
| "L"
| "M"
| "N"
| "O"
| "P"
| "Q"
| "R"
| "S"
| "T"
| "U"
| "V"
| "W"
| "X"
| "Y"
| "Z";
export type SpecialKey = "ArrowUp" | "ArrowDown" | "...more keys";
export type KeyboardKey = NumericKey | AlphabeticKey | SpecialKey;
export type OnyxKeyProps = {
key: KeyboardKey;
}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.
Brilliant idea :)
I was thinking of providing a constant that we would recommend using as a source of truth for keys with the possibility of having some of our own keys and about our own type in the same way as you described as well.
I don't really like the constant, since it's an additional import and an additional variable whose name you always need to keep in mind. So I would avoid using it.
Yes, own type gives a very good DX due to autocomplete, 100% agree.
But support may indeed be an issue. Despite this, in the context of a component like shortcut, this doesn't sound like the end of the world, the most important keys can be declared in the type.
| import OnyxShortcut from "./OnyxShortcut.vue"; | ||
|
|
||
| const meta: Meta<typeof OnyxShortcut> = { | ||
| title: "Feedback/Shortcut", |
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.
| title: "Feedback/Shortcut", | |
| title: "Basic/Shortcut", |
We intended to move the shortcut component to the "Basic" category as its use-case can vary so we found "Basic" the most appropriate one. What do you think?
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.
I simply consider anything that asks the user to take some action as "feedback".
Anyway I would move it simply to "Basic")
|
This PR will be heavily modified within a week, so don't waste time reviewing it for now. |
Relates to #2066
Description
OnyxKeyandOnyxShortcutcomponentsuseShortcutSequence- composable for detecting multi-step keyboard sequencesshortcut.ts- utilities for canonical key mapping andKeyboardEventhandlingdetectOperatingSystem- auto-detect OS for appropriate key stylingKey Features
Ctrl+K → GChecklist
npx changeset addif your changes should be released as npm package (because they affect the library usage)apps/docs) if needed