Svelte: implement keyboard shortcut design#63253
Conversation
camdencheek
force-pushed
the
cc/kbd-design
branch
from
dd562c1 to
26fc40b
Compare
camdencheek
force-pushed
the
cc/kbd-design
branch
from
953c00f to
e84d2a1
Compare
| import { formatShortcutParts, type Keys } from '$lib/Hotkey' | ||
|
|
||
| export let shorcut: Keys | ||
| export let shortcut: Keys |
| // NOTE: the height of the kdb element is based on the base | ||
| // line height. There is no way to query the line height of the | ||
| // parent, so we assume that the container has the standard | ||
| // line height. In the case the parent has a line height of 1, | ||
| // this will grow the container slightly. This can be overridden | ||
| // by setting `--line-height-base` if overriding it in the parent. |
There was a problem hiding this comment.
This is a bit of a gotcha, but the shortcuts look pretty bad if we constrain them to fit inside a line-height: 1.
| // When inside a selected container, show the selected variant | ||
| :global([aria-selected='true']) & { | ||
| color: white; | ||
| background-color: var(--primary); | ||
| } |
There was a problem hiding this comment.
What do we think about this pattern? Rather than overriding from the container that is selected with something like :global(kbd), the KeyboardShortcut component controls its styling and variants are activated by selectors on parent components. This effectively makes aria-selected='true' part of fthe public API of this component, which we would probably want to document loudly. But, I like that this is a pure-CSS solution that doesn't require prop-drilling variables through components.
There was a problem hiding this comment.
In general I like this. I aligns with my expectation that components should be self contained and in control over their style. It also makes things consistent because you can't forget to style it differently or pass a prop.
It might not work in all situations though. E.g. maybe we'll have a selected element with multiple shortcuts in it and we don't want to style them differently. But I'm sure we find a solution should that be the case.
fkling
left a comment
fkling
left a comment
There was a problem hiding this comment.
Thank you for cleaning up all the other keyboard shortcut instances!
| // When inside a selected container, show the selected variant | ||
| :global([aria-selected='true']) & { | ||
| color: white; | ||
| background-color: var(--primary); | ||
| } |
There was a problem hiding this comment.
In general I like this. I aligns with my expectation that components should be self contained and in control over their style. It also makes things consistent because you can't forget to style it differently or pass a prop.
It might not work in all situations though. E.g. maybe we'll have a selected element with multiple shortcuts in it and we don't want to style them differently. But I'm sure we find a solution should that be the case.
2 participants
The current keyboard shortcuts look pretty funky against the sleek new design IMO, so I took a few minutes (that turned into a couple hours) to implement the keyboard shortcut design from Figma.
Test plan
I manually checked every place we add keyboard shortcuts. Screenshots below.