feat(select): add hotkey prop for items & improve keyboard navigation#3990
feat(select): add hotkey prop for items & improve keyboard navigation#3990
hotkey prop for items & improve keyboard navigation#3990Conversation
📝 WalkthroughWalkthroughThe pull request introduces hotkey support to the Select component by extending the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Document
participant SelectComponent
participant OptionMatcher
participant MenuItems
User->>Document: Press hotkey
Document->>SelectComponent: keydown event (menuOpen)
activate SelectComponent
SelectComponent->>SelectComponent: Check if reserved hotkey
SelectComponent->>OptionMatcher: Normalize hotkey & find match
OptionMatcher->>OptionMatcher: Search enabled options by normalized hotkey
OptionMatcher-->>SelectComponent: Matching option found
SelectComponent->>SelectComponent: Update internal state / emit change
deactivate SelectComponent
SelectComponent->>MenuItems: Render selected option
MenuItems-->>User: Visual feedback
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 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-3990/ |
Kiarokh
force-pushed
the
split/3.5-improve-visual-style
branch
from
c53fa25 to
4ae8884
Compare
Kiarokh
force-pushed
the
split/4-select-hotkeys
branch
from
abcf148 to
6cdecc7
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: 1
🤖 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/select.tsx`:
- Around line 441-443: The selection path emits change and then closes the menu
without cancelling pending focus tasks; update the block that calls
this.change.emit(matchedOption), this.menuOpen = false, this.setTriggerFocus()
to first call cancelPendingFocus() (the same helper used in closeMenu() and
handleMenuChange()) before flipping menuOpen and setting trigger focus so any
IntersectionObserver/timeouts are cleared.
🪄 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: 5c840184-08d6-41e8-ade6-13b34615bdc0
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (4)
src/components/select/examples/select-hotkeys.tsxsrc/components/select/option.types.tssrc/components/select/select.template.tsxsrc/components/select/select.tsx
| this.change.emit(matchedOption); | ||
| this.menuOpen = false; | ||
| this.setTriggerFocus(); |
There was a problem hiding this comment.
Missing cancelPendingFocus() call before closing menu.
When selecting via hotkey in single-select mode, cancelPendingFocus() is not called before setting menuOpen = false. Compare with closeMenu() (line 537-541) and handleMenuChange() (line 397) which both call cancelPendingFocus(). This could leave a stale IntersectionObserver or timeout running.
Proposed fix
this.change.emit(matchedOption);
this.menuOpen = false;
+ this.cancelPendingFocus();
this.setTriggerFocus();📝 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.
| this.change.emit(matchedOption); | |
| this.menuOpen = false; | |
| this.setTriggerFocus(); | |
| this.change.emit(matchedOption); | |
| this.menuOpen = false; | |
| this.cancelPendingFocus(); | |
| this.setTriggerFocus(); |
🤖 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 441 - 443, The selection path
emits change and then closes the menu without cancelling pending focus tasks;
update the block that calls this.change.emit(matchedOption), this.menuOpen =
false, this.setTriggerFocus() to first call cancelPendingFocus() (the same
helper used in closeMenu() and handleMenuChange()) before flipping menuOpen and
setting trigger focus so any IntersectionObserver/timeouts are cleared.
Consolidated PR Review -- 6 DimensionsPR SummaryThis PR adds keyboard shortcut ( 1. Backward Compatibility -- GOOD ✅The changes are purely additive. The new
2. Code Quality -- ACCEPTABLE ✅The implementation follows established patterns from
3. Architecture -- GOOD ✅The feature integrates cleanly with the existing architecture, reusing
4. Security -- SAFE ✅No significant security issues found. The hotkey string flows through normalization/tokenization and is rendered via Stencil's JSX (virtual DOM), not
5. Observability -- SAFE ✅No significant issues found. This is purely front-end UI component logic with no server-side code, logging, metrics, or network calls.
6. Performance -- GOOD ✅No blocking performance issues. The implementation is well-considered for typical option counts.
Overall Verdicts
Top Recommendations
🤖 Generated with Claude Code |
| this.setTriggerFocus(); | ||
| }; | ||
|
|
||
| private isReservedSelectHotkey(hotkey: string): boolean { |
There was a problem hiding this comment.
@Kiarokh Small observation on isReservedSelectHotkey vs isReservedMenuHotkey:
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 ctrl+arrowdown would work in a menu but be blocked in a select. Since unmodified arrows are used for dropdown navigation in both components equally, it seems like the logic should be the same — i.e., the select version should also do the early if (hasModifiers) return false check, matching the menu.
Was this divergence intentional, or should they be unified?
There was a problem hiding this comment.
Kia answered in Slack. The divergence is unintentional, so we should break out a helper and use it in both places.
There was a problem hiding this comment.
yes, they both can share the same logic 😊
2 participants
fix: https://github.com/Lundalogik/crm-client/issues/755
Summary by CodeRabbit
New Features
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: