feat(select): add hotkey prop for items & improve keyboard navigation#3979
feat(select): add hotkey prop for items & improve keyboard navigation#3979Kiarokh wants to merge 1 commit intosplit/3.5-improve-visual-stylefrom
hotkey prop for items & improve keyboard navigation#3979Conversation
📝 WalkthroughWalkthroughAdds hotkey support to the limel-select component (option-level Changes
Sequence DiagramsequenceDiagram
participant User
participant Document
participant Select
participant Options
participant Menu
User->>Document: press key
Document->>Select: document keydown
Select->>Select: normalize hotkey (cache)
Select->>Select: validate (menu open, not mobile, not repeat, not reserved)
Select->>Options: find option matching hotkey
alt match found and enabled
Options-->>Select: matched option
Select->>Select: update selection (single/multiple)
Select->>Menu: emit change
Select->>Select: close menu (if single) and restore focus
else no match / invalid
Select-->>Document: ignore
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3979/ |
adrianschmidt
force-pushed
the
split/3-menu-hotkeys
branch
3 times, most recently
from
e6762e5 to
7326c80
Compare
adrianschmidt
force-pushed
the
split/4-select-hotkeys
branch
from
2d151c9 to
6b9083e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/markdown/partial-styles/_kbd.scss`:
- Around line 31-33: The rule using &::first-letter in
src/components/markdown/partial-styles/_kbd.scss uppercases the first character
of <kbd> labels and can misrepresent keyboard shortcuts; remove the
&::first-letter { text-transform: uppercase; } block (or adjust scope so it does
not apply to kbd elements) so that <kbd> content is rendered exactly as authored
rather than being style-transformed.
In `@src/components/select/examples/select-hotkeys.tsx`:
- Around line 86-88: The property handleChange is defined as an arrow function
and never reassigned, so mark it readonly to satisfy static analysis: add the
readonly modifier to the handleChange property declaration (private readonly
handleChange = (event: LimelSelectCustomEvent<Option>) => { this.value =
event.detail; }); to ensure immutability and silence the linter while keeping
the existing behavior.
- Line 42: The class field options is never reassigned so mark it readonly to
express immutability; update the declaration of options (the private options:
(Option | ListSeparator)[] = [...]) to private readonly options: (Option |
ListSeparator)[] = [...] so the compiler and readers know it won't be reassigned
and to satisfy the static analysis warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37274e04-81bc-4478-a385-72704443191d
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (5)
src/components/markdown/partial-styles/_kbd.scsssrc/components/select/examples/select-hotkeys.tsxsrc/components/select/option.types.tssrc/components/select/select.template.tsxsrc/components/select/select.tsx
| &::first-letter { | ||
| text-transform: uppercase; | ||
| } |
There was a problem hiding this comment.
Avoid transforming <kbd> content with ::first-letter
Uppercasing the first letter changes the rendered key label and can misrepresent actual shortcuts/commands (e.g., case-sensitive keys or multi-key sequences). Prefer rendering the intended label directly from content instead of style-based mutation.
Suggested fix
- &::first-letter {
- text-transform: uppercase;
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &::first-letter { | |
| text-transform: uppercase; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/markdown/partial-styles/_kbd.scss` around lines 31 - 33, The
rule using &::first-letter in src/components/markdown/partial-styles/_kbd.scss
uppercases the first character of <kbd> labels and can misrepresent keyboard
shortcuts; remove the &::first-letter { text-transform: uppercase; } block (or
adjust scope so it does not apply to kbd elements) so that <kbd> content is
rendered exactly as authored rather than being style-transformed.
There was a problem hiding this comment.
I explained this in details in the original PR once. But can't find it right now to link to it. Maybe @coderabbitai remembers?
Short answer (which I hope is enough):
I don't think the feedback is valid. All physical keyboards have the capital letter printed on their keys. Apple keybaords write modifier key names and esc and such with small letters. But that's alright too if we visualize their first letters with capital letters as well. We render glyphs anywas.
@coderabbitai what do you mean by "case-sensitive keys or multi-key sequences" how rendering capital letters can be misinterpreted by users?
There was a problem hiding this comment.
@Kiarokh, I wasn't able to locate the original PR discussion where this was explained in detail, sorry about that.
To clarify what I meant by the original concern:
- "Case-sensitive keys" — if an author intentionally writes
<kbd>a</kbd>vs<kbd>A</kbd>to visually distinguish a lowercase key from an uppercase one (e.g., to describe a case-sensitive terminal shortcut), the::first-letterrule would render both asA, erasing that distinction. That was the scenario I had in mind. - "Multi-key sequences" — if someone wrote a sequence like
<kbd>ctrl+shift+a</kbd>in a single element, only the first lettercwould be capitalized, yieldingCtrl+shift+a. However, that's a misuse of<kbd>(the spec recommends nesting<kbd>elements for sequences), so it's more of an edge case.
That said, your reasoning is compelling: the visual metaphor here is a physical key cap, and physical keyboards universally display uppercase letters. Capitalizing ctrl → Ctrl, shift → Shift, esc → Esc is entirely consistent with that metaphor. Given this context and the fact that the ::first-letter rule only uppercases the first character (not the full label), I agree my concern was overstated for this particular use case. I'm happy to withdraw it.
✏️ Learnings added
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 3979
File: src/components/markdown/partial-styles/_kbd.scss:31-33
Timestamp: 2026-03-30T16:57:04.211Z
Learning: In lime-elements, `kbd` styling in markdown (src/components/markdown/partial-styles/_kbd.scss) intentionally uses `&::first-letter { text-transform: uppercase; }` to mirror physical keyboard key cap styling, where keys are displayed with uppercase labels. This is a deliberate visual design decision and not a bug — the metaphor is a physical key cap, not raw text content.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 3658
File: src/components/profile-picture/profile-picture.scss:120-124
Timestamp: 2025-09-03T07:17:23.267Z
Learning: Issue `#3666` in the lime-elements repository tracks the removal of a CSS hack in profile-picture component that overrides icon color via --limel-theme-on-surface-color, to be replaced with proper icon color support in limel-icon-button.
Learnt from: adrianschmidt
Repo: Lundalogik/lime-elements PR: 3960
File: src/style-utilities.md:31-33
Timestamp: 2026-03-19T19:28:52.281Z
Learning: When reviewing Dart Sass (SCSS) imports, distinguish between the two package-resolution mechanisms:
1) Use the `pkg:` URL form (e.g., `use "pkg:scope/name"`) only when the Sass plugin/CLI is configured with `NodePackageImporter`, which uses the target package’s `package.json` `sass`/`exports` fields.
2) Prefer/expect `includePaths` for load-path resolution: if build config sets `includePaths` (e.g., `['node_modules']`), then plain `use 'scope/name'` should resolve via standard file resolution within the resolved directory—typically `index.scss` or `_index.scss` (e.g., `node_modules/scope/name/index.scss`)—without needing `NodePackageImporter` or the `pkg:` prefix.
There was a problem hiding this comment.
@coderabbitai I see what you mean. Since this style is used in limel-markdown as well (not just limel-hotkey) we should be more careful. I'm gonna add a fixup
adrianschmidt
force-pushed
the
split/4-select-hotkeys
branch
from
6b9083e to
b0743eb
Compare
Consolidated PR Review -- 6 DimensionsPR SummaryPR #3979 adds 1. Backward Compatibility -- SAFE ✅All changes are purely additive. The new
2. Code Quality -- ACCEPTABLE ✅Well-implemented following patterns established in PR #3978 (menu hotkeys). The main concerns are duplicated logic and missing tests.
3. Architecture -- ACCEPTABLE ✅The select component follows the same hotkey architecture as the menu component, reusing the shared
4. Security -- SAFE ✅No significant security issues found. The
5. Observability -- GOOD ✅No significant observability concerns. Appropriate for a UI component library.
6. Performance -- GOOD ✅No performance blockers. The caching strategy is sound.
Overall Verdicts
Top Recommendations
|
adrianschmidt
force-pushed
the
split/4-select-hotkeys
branch
from
b0743eb to
abcf148
Compare
|
@Kiarokh Small observation on In menu, the logic is: if (hasModifiers) {
return false; // any modified key is allowed
}
return key === 'arrowup' || ... || key === 'tab' || key === 'enter' || key === 'space' || key === 'escape';In select, arrow keys and tab are reserved regardless of modifiers: if (key === 'arrowup' || ... || key === 'arrowright') {
return true; // always reserved, even with modifiers
}
if (key === 'tab') {
return true; // always reserved, even with modifiers
}
const hasModifiers = tokens.length > 1;
return !hasModifiers && (key === 'enter' || key === 'space' || key === 'escape');This means a hotkey like Was this divergence intentional, or should they be unified? |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/select/examples/select-hotkeys.tsx`:
- Around line 64-69: The demo option object that sets hotkey: 'cmd+d' (the entry
with value 'closed' / text 'Closed') uses a meta-based shortcut which normalizes
to OS-level shortcuts on Windows/Linux; update that hotkey to a portable,
non-meta combination (for example 'ctrl+shift+d' or another ctrl/alt-based
combo) so the example works consistently across platforms and avoids OS shortcut
collisions—locate the object with value 'closed' in select-hotkeys.tsx and
replace its hotkey property accordingly.
In `@src/components/select/select.tsx`:
- Around line 424-438: The multi-select branch currently calls
this.change.emit(nextValue) directly (inside the this.multiple / isMultiple /
matchedOption block), which bypasses the scroll snapshot/restore logic in
handleMenuChange() and causes the menu to jump; instead, route the update
through the existing handleMenuChange flow (call the appropriate
handleMenuChange path or helper with nextValue so the snapshot/restore logic
runs) rather than emitting nextValue directly, ensuring you reference the same
symbols (this.multiple, isMultiple(this.value), matchedOption, nextValue) when
making the change.
- Around line 473-485: findOptionByHotkey currently iterates
getOptionsExcludingSeparators() and may match options that createMenuItems
intentionally omits when this.selectProps.required is true (e.g.,
empty-text/placeholder options). Update findOptionByHotkey to apply the same
filtering logic used in createMenuItems: skip any option that would be filtered
out when this.selectProps.required is true (for example, options with empty
text/value or the placeholder option), then normalize and compare hotkeys as
before; reference the findOptionByHotkey, getOptionsExcludingSeparators, and
createMenuItems logic to ensure the filters are identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5fc9624d-e830-4634-9a11-e22f65630dbf
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (5)
src/components/markdown/partial-styles/_kbd.scsssrc/components/select/examples/select-hotkeys.tsxsrc/components/select/option.types.tssrc/components/select/select.template.tsxsrc/components/select/select.tsx
| { | ||
| text: 'Closed', | ||
| secondaryText: 'as not planned', | ||
| value: 'closed', | ||
| hotkey: 'cmd+d', | ||
| }, |
There was a problem hiding this comment.
Replace cmd+d with a portable demo shortcut.
cmd+d normalizes to meta+d, which becomes Win+D on Windows/Linux and collides with an OS-owned shortcut. That makes the example unreliable outside macOS; please use a non-meta combination here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/select/examples/select-hotkeys.tsx` around lines 64 - 69, The
demo option object that sets hotkey: 'cmd+d' (the entry with value 'closed' /
text 'Closed') uses a meta-based shortcut which normalizes to OS-level shortcuts
on Windows/Linux; update that hotkey to a portable, non-meta combination (for
example 'ctrl+shift+d' or another ctrl/alt-based combo) so the example works
consistently across platforms and avoids OS shortcut collisions—locate the
object with value 'closed' in select-hotkeys.tsx and replace its hotkey property
accordingly.
| if (this.multiple) { | ||
| const currentValue = isMultiple(this.value) ? this.value : []; | ||
| const hasSelectedOption = currentValue.some( | ||
| (option) => option.value === matchedOption.value | ||
| ); | ||
|
|
||
| const nextValue = hasSelectedOption | ||
| ? currentValue.filter( | ||
| (option) => option.value !== matchedOption.value | ||
| ) | ||
| : [...currentValue, matchedOption]; | ||
|
|
||
| this.change.emit(nextValue); | ||
|
|
||
| return; |
There was a problem hiding this comment.
Keep the multi-select hotkey path on the existing scroll-preserving flow.
handleMenuChange() already snapshots and restores the menu surface scroll position for multi-select updates because rerendering otherwise jumps the menu back to the top. This new branch emits nextValue directly, so toggling items with hotkeys will regress that behavior in long dropdowns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/select/select.tsx` around lines 424 - 438, The multi-select
branch currently calls this.change.emit(nextValue) directly (inside the
this.multiple / isMultiple / matchedOption block), which bypasses the scroll
snapshot/restore logic in handleMenuChange() and causes the menu to jump;
instead, route the update through the existing handleMenuChange flow (call the
appropriate handleMenuChange path or helper with nextValue so the
snapshot/restore logic runs) rather than emitting nextValue directly, ensuring
you reference the same symbols (this.multiple, isMultiple(this.value),
matchedOption, nextValue) when making the change.
| private findOptionByHotkey(pressedHotkey: string): Option | null { | ||
| for (const option of this.getOptionsExcludingSeparators()) { | ||
| if (option.disabled || !option.hotkey) { | ||
| continue; | ||
| } | ||
|
|
||
| const normalized = this.getNormalizedHotkey(option.hotkey); | ||
| if (normalized && normalized === pressedHotkey) { | ||
| return option; | ||
| } | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Hotkeys can still select options that the required menu intentionally hides.
findOptionByHotkey() scans every non-separator option, but createMenuItems() filters out empty-text options when required is true. A hidden “empty option” can therefore still be selected via hotkey, which breaks the required-select invariant. Please apply the same filter before matching here.
🔧 Suggested direction
private findOptionByHotkey(pressedHotkey: string): Option | null {
- for (const option of this.getOptionsExcludingSeparators()) {
+ const options = this.required
+ ? this.getOptionsExcludingSeparators().filter((option) => option.text)
+ : this.getOptionsExcludingSeparators();
+
+ for (const option of options) {
if (option.disabled || !option.hotkey) {
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private findOptionByHotkey(pressedHotkey: string): Option | null { | |
| for (const option of this.getOptionsExcludingSeparators()) { | |
| if (option.disabled || !option.hotkey) { | |
| continue; | |
| } | |
| const normalized = this.getNormalizedHotkey(option.hotkey); | |
| if (normalized && normalized === pressedHotkey) { | |
| return option; | |
| } | |
| } | |
| return null; | |
| private findOptionByHotkey(pressedHotkey: string): Option | null { | |
| const options = this.required | |
| ? this.getOptionsExcludingSeparators().filter((option) => option.text) | |
| : this.getOptionsExcludingSeparators(); | |
| for (const option of options) { | |
| if (option.disabled || !option.hotkey) { | |
| continue; | |
| } | |
| const normalized = this.getNormalizedHotkey(option.hotkey); | |
| if (normalized && normalized === pressedHotkey) { | |
| return option; | |
| } | |
| } | |
| return null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/select/select.tsx` around lines 473 - 485, findOptionByHotkey
currently iterates getOptionsExcludingSeparators() and may match options that
createMenuItems intentionally omits when this.selectProps.required is true
(e.g., empty-text/placeholder options). Update findOptionByHotkey to apply the
same filtering logic used in createMenuItems: skip any option that would be
filtered out when this.selectProps.required is true (for example, options with
empty text/value or the placeholder option), then normalize and compare hotkeys
as before; reference the findOptionByHotkey, getOptionsExcludingSeparators, and
createMenuItems logic to ensure the filters are identical.
kbd elements
adrianschmidt
changed the title
hotkey prop for items & improve keyboard navigation
|
This is messy with reviews for commits no longer part of the PR. I'll close this so we can open a new fresh PR instead. |
2 participants
Summary by CodeRabbit
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: