Conversation
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3818/ |
📝 WalkthroughWalkthroughAdds first-class hotkey support: new Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(226, 240, 255, 0.5)
actor User
end
participant Surface as MenuSurface
participant Menu as LimelMenu
participant Utils as HotkeyUtils
participant Items as MenuItems
participant Evt as SelectEvent
User->>Surface: press keyboard (e.g. Alt+C)
Surface->>Menu: forward keydown to surface handler
Menu->>Utils: isKeyboardEventFromTextInput(event)?
Utils-->>Menu: false
Menu->>Utils: hotkeyFromKeyboardEvent(event)
Utils-->>Menu: "alt+c"
Menu->>Menu: isReservedMenuHotkey? (no)
Menu->>Items: find visible item with hotkey "alt+c"
Items-->>Menu: matching item found
Menu->>Surface: event.preventDefault()
Menu->>Evt: emit select for matched item
Evt-->>User: item selected / menu closed
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 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 |
Kiarokh
force-pushed
the
menu-hotkeys
branch
2 times, most recently
from
e2c148e to
530a80c
Compare
Kiarokh
added
feature
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/menu/examples/menu-hotkeys.tsx (1)
73-88: 🛠️ Refactor suggestion | 🟠 MajorWrap multiple JSX elements in
<Host>instead of an array literal.Per the coding guidelines for StencilJS
.tsxfiles, when returning multiple top-level JSX elements from therendermethod, they should be wrapped in the special<Host>element rather than an array literal.♻️ Proposed fix
+import { Host } from '@stencil/core';public render() { - return [ + return ( + <Host> + <limel-menu items={this.items} onSelect={this.handleSelect}> + <limel-icon-button + label="Menu" + icon="menu_2" + elevated={true} + slot="trigger" + /> + </limel-menu> + <limel-example-value + label="Last selected item" + value={this.lastSelectedItem} + /> + </Host> + ); - <limel-menu items={this.items} onSelect={this.handleSelect}> - <limel-icon-button - label="Menu" - icon="menu_2" - elevated={true} - slot="trigger" - /> - </limel-menu>, - <limel-example-value - label="Last selected item" - value={this.lastSelectedItem} - />, - ]; }As per coding guidelines: "When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element."
🤖 Fix all issues with AI agents
In `@src/components/menu/menu.tsx`:
- Around line 367-385: In findMenuItemByHotkey, remove the redundant type cast
by relying on the isMenuItem type guard: replace the line that assigns rawHotkey
= (item as MenuItem).hotkey with a direct access const rawHotkey = item.hotkey;
(keep the existing checks using this.isMenuItem, this.getNormalizedHotkey,
item.disabled and this.visibleItems unchanged).
In `@src/util/hotkeys.ts`:
- Around line 47-55: The return logic in the normalization branch is redundant:
after checking if /^f\d{1,2}$/.test(normalized) and returning normalized, the
subsequent if (normalized.length === 1) branch also returns normalized and the
final return returns normalized again; simplify by removing the length check and
directly returning normalized if the regex test fails. Update the hotkey
normalization function to keep the /^f\d{1,2}$/ early return and replace the
remaining branches with a single return normalized to preserve existing behavior
(refer to the normalized variable and the regex check).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/menu/menu.e2e.ts`:
- Around line 265-306: Tests use fixed waits (page.waitForTimeout(200/50)),
causing flakiness; replace those with deterministic checks: after
limelMenu.setProperty('items') and trigger.click(), wait for the menu to be open
or the focused item to appear (for example, await page.waitForFunction to assert
trigger.getAttribute('aria-expanded') === 'true' or wait for a focused menu item
via page.find('li[aria-selected="true"], li:focus') ) before pressing keys with
page.keyboard.press('Enter'/'ArrowDown'), and remove the waitForTimeout calls so
selection assertions (spy on 'select') run only after the menu/focus state is
confirmed.
In `@src/util/hotkeys.ts`:
- Around line 203-231: The isKeyboardEventFromTextInput function currently
treats missing composedPath() as an empty path and can miss text inputs; update
isKeyboardEventFromTextInput to fall back to a single-node path using
event.target (and/or event.srcElement for legacy IE) when composedPath is not
available, then run the same checks (isContentEditable, tagName
INPUT/TEXTAREA/SELECT, role="textbox") against that fallback node so hotkeys
won't fire while typing in unsupported browsers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/hotkey/hotkey.tsx`:
- Around line 66-73: normalizeHotkeyString currently splits on "+" and drops
empty tokens so a lone this.value of "+" becomes null and can never match
hotkeyFromKeyboardEvent; fix by special-casing the exact string "+" inside
normalizeHotkeyString (or the caller before normalizing) to return the canonical
token that normalizeKey expects (e.g., map "+" to the supported token name) so
pressed comparisons work, or alternatively update the docs/examples to use the
supported token instead of "+"; ensure the mapping uses the same normalized form
returned by normalizeKey so the pressed === expected check succeeds.
dd96fcb to
1a3d2c4
Compare
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/hotkey/hotkey.tsx`:
- Around line 55-61: The mapped <kbd> elements rendered in parts.map(...) lack a
key which breaks the useJsxKeyInIterable lint rule; update the map callback in
the render (the parts.map producing <kbd> and calling this.formatDisplayToken)
to provide a unique key for each <kbd> — e.g., use the part value or an index
fallback (key={part} or key={`${part}-${i}`}) so each <kbd> inside Host has a
stable key.
In `@src/util/device.ts`:
- Around line 49-60: The return expression in isAppleDevice() is redundant
because isIPadOS13Plus is defined as (isMacLike && getMaxTouchPoints() > 1), so
including it separately is unnecessary; update the function to simplify the
final return to "isIPadIPhoneIPod || isMacLike" (or remove isIPadOS13Plus usage
while keeping the variable for documentation if preferred) and ensure variables
getUserAgent(), getPlatform(), isIPadIPhoneIPod, isMacLike, and isIPadOS13Plus
are left intact for clarity.
In `@src/util/hotkeys.spec.ts`:
- Around line 14-40: Add unit tests for hotkeyFromKeyboardEvent to cover
modifier combinations, function keys, arrow keys, and code-based normalization:
add a test like "includes multiple modifiers" that asserts
hotkeyFromKeyboardEvent returns "ctrl+alt+a" for an event with key 'a' and
ctrlKey/altKey true; add "handles function keys" asserting 'f1' and 'f12' for
events with key 'F1'/'F12'; add "handles arrow keys" asserting
'arrowleft'/'arrowright' for ArrowLeft/ArrowRight keys; and add "uses event.code
for non-QWERTY" that simulates a non-letter key where key differs from code
(e.g., key='@' code='Quote' or code-based letter) to ensure the function
normalizes using event.code when appropriate by comparing expected normalized
output from hotkeyFromKeyboardEvent.
| export function isAppleDevice(): boolean { | ||
| const ua = getUserAgent(); | ||
| const platform = getPlatform(); | ||
|
|
||
| const isIPadIPhoneIPod = /iPad|iPhone|iPod/i.test(ua); | ||
| const isMacLike = /Mac/i.test(platform); | ||
|
|
||
| // iPadOS can report itself as Mac; use touch points to detect it. | ||
| const isIPadOS13Plus = isMacLike && getMaxTouchPoints() > 1; | ||
|
|
||
| return isIPadIPhoneIPod || isIPadOS13Plus || isMacLike; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the return expression.
The logic is correct, but isIPadOS13Plus is redundant in the return statement since it implies isMacLike by definition (isIPadOS13Plus = isMacLike && ...). The expression isIPadIPhoneIPod || isIPadOS13Plus || isMacLike is equivalent to isIPadIPhoneIPod || isMacLike.
That said, keeping isIPadOS13Plus explicit could be intentional for documentation purposes—it makes the three detection cases clearer to readers.
♻️ Optional simplification
- return isIPadIPhoneIPod || isIPadOS13Plus || isMacLike;
+ // iPadOS13Plus is a subset of isMacLike, so we can simplify:
+ return isIPadIPhoneIPod || isMacLike;Or keep as-is if the explicit enumeration of cases is preferred for readability.
📝 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.
| export function isAppleDevice(): boolean { | |
| const ua = getUserAgent(); | |
| const platform = getPlatform(); | |
| const isIPadIPhoneIPod = /iPad|iPhone|iPod/i.test(ua); | |
| const isMacLike = /Mac/i.test(platform); | |
| // iPadOS can report itself as Mac; use touch points to detect it. | |
| const isIPadOS13Plus = isMacLike && getMaxTouchPoints() > 1; | |
| return isIPadIPhoneIPod || isIPadOS13Plus || isMacLike; | |
| } | |
| export function isAppleDevice(): boolean { | |
| const ua = getUserAgent(); | |
| const platform = getPlatform(); | |
| const isIPadIPhoneIPod = /iPad|iPhone|iPod/i.test(ua); | |
| const isMacLike = /Mac/i.test(platform); | |
| // iPadOS can report itself as Mac; use touch points to detect it. | |
| const isIPadOS13Plus = isMacLike && getMaxTouchPoints() > 1; | |
| // iPadOS13Plus is a subset of isMacLike, so we can simplify: | |
| return isIPadIPhoneIPod || isMacLike; | |
| } |
🤖 Prompt for AI Agents
In `@src/util/device.ts` around lines 49 - 60, The return expression in
isAppleDevice() is redundant because isIPadOS13Plus is defined as (isMacLike &&
getMaxTouchPoints() > 1), so including it separately is unnecessary; update the
function to simplify the final return to "isIPadIPhoneIPod || isMacLike" (or
remove isIPadOS13Plus usage while keeping the variable for documentation if
preferred) and ensure variables getUserAgent(), getPlatform(), isIPadIPhoneIPod,
isMacLike, and isIPadOS13Plus are left intact for clarity.
a69816e to
bf75cb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/hotkey/hotkey.tsx`:
- Around line 55-61: The kbd elements rendered in the Host mapping currently set
aria-hidden="true" which triggers the a11y lint rule; remove the aria-hidden
attribute from the <kbd> elements in the parts.map render soScreen readers can
access the keys while keeping the Host's aria-label (refer to Host, parts.map
and formatDisplayToken in hotkey.tsx to locate the code). Ensure no other
aria-hidden is added and that formatDisplayToken continues to produce the
visible label for each kbd.
In `@src/components/menu/menu.e2e.ts`:
- Around line 362-381: The test dispatches the KeyboardEvent to
limel-menu-surface immediately after clicking the trigger, which can race if the
surface hasn't opened; after clicking the trigger (the variable trigger and the
call to trigger.click()/page.waitForChanges()), wait deterministically for the
surface to exist and be open/visible (e.g., use page.waitForSelector or a short
polling loop to confirm document.querySelector('limel-menu-surface') is present
and has the open/visible state) before executing the evaluated block that
dispatches the KeyboardEvent; update the code that finds limel-menu-surface and
dispatches the event (the evaluate callback) to run only once that deterministic
wait completes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/hotkey/hotkey.tsx`:
- Around line 78-83: The UI shows 'meta' as 'Ctrl' for non-Apple (case 'meta' /
'cmd' / 'command' returning isApple ? '⌘' : 'Ctrl') but matching still expects
Meta, so fix by normalizing the incoming hotkey tokens when not on Apple: in the
hotkey normalization function (the code that processes 'lower' tokens before
matching, e.g., where you switch on lower) map 'meta'/'cmd'/'command' → 'ctrl'
for non-Apple platforms so both display and matching align (alternatively, if
you prefer to keep matching as Meta, change the display branch to render
'Meta'/'Win' instead—choose one consistent approach and apply it in the switch
handling and the hotkey normalization logic).
src/components/hotkey/hotkey.tsx
Outdated
| ); | ||
| } | ||
|
|
||
| private formatDisplayToken(token: string, isApple: boolean): string { |
There was a problem hiding this comment.
Does this need to live inside the component? We can move it to a separate function for easier testing
There was a problem hiding this comment.
it could, but I also wanted to use it to add a class to render the glyphs a bit larger than the letters. added here: 195e489
Is it a big deal?
There was a problem hiding this comment.
No it's not 😄 Just separation of concerns
| if ((event as any)[Hotkey.handledFlag]) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why do we need this check? Do we expect the event to propagate though multiple hotkeys? It does not look like they will ever be nested inside each other
There was a problem hiding this comment.
I added it because of some discussions I had with a 🤖 friend, when trying to mitigate buggy behaviours, due to having multiple instances of the same component potentially rendered all at once.
Asked it to explain, and it said:
That check is there to prevent duplicate handling of the same physical keypress when there are multiple limel-hotkey instances on the page, not because they’re nested.
What happens otherwise:
Every
limel-hotkeyregisters@Listen('keydown', { target: 'document', capture: true }).
A single keydown will invokehandleKeyDown()once per instance.
If you render several hotkeys (like in the example, or a menu with many items), each instance will evaluate the event; without a guard, more than one instance can emit if they happen to match (or if you accidentally have duplicates).
Why a flag on the event:
stopPropagation()isn’t ideal here because we’re listening on document in capture and we don’t want to interfere with other global handlers in the app.
The per-event flag makes it “handled once” across alllimel-hotkeylisteners, regardless of render order.
So yes: we do expect the same event to “reach” multiple hotkey components because they all listen globally, even though they aren’t nested.If you’re confident you’ll never have duplicate hotkeys active at the same time, you could remove it—but in menus/examples it’s very easy to have multiple limel-hotkey elements present, so it’s a safety net.
There was a problem hiding this comment.
But with this check, it will be unclear which of those hotkeys will be triggered. There is no way for the consumer to know if their instance will be allowed or blocked by this check. Is that the behavior we want? The way it will work now will most likely be the component that renders first is the one that will receive the event
There was a problem hiding this comment.
Hm… u r right.
what's a better approach? Just remove it?
There was a problem hiding this comment.
Yeah I think we should remove it. What does the 🤖 say?
There was a problem hiding this comment.
Now I've kept the mechanics to trigger a console warning when a duplicate hotkey is identified. ec3a5c0
This way, the limel-hotkey component will only work for the first instance, and reduces a little bit of the confusion risk for the end-user. And the consumer can hopefully see a warning in the console.
Most of this responsibility is on the consumer anyway. But it's good to have some error prevention in the component itself as well.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/hotkey/examples/hotkey-basic.tsx`:
- Around line 15-24: Typo: the docs line "`cmd` or `command` are is just an
alias for `meta`" contains a duplicated word; update the sentence in the hotkey
example text so it reads correctly (e.g., "`cmd` or `command` is just an alias
for `meta`" or "`cmd` and `command` are just aliases for `meta`")—look for the
string containing `cmd`, `command`, and `meta` in hotkey-basic.tsx and fix the
duplicated "are is" to the chosen correct phrasing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/menu/menu.e2e.ts`:
- Around line 203-285: Duplicate logic in waitForFocusedMenuItemIndex: extract
the core DOM-inspection code from getIndex into a single serializable function
(e.g., const findFocusedIndex = function() { ... } or a template literal string)
and reuse that in both the page.evaluate(getIndex) call and the
anyPage.waitForFunction(...) call; update waitForFocusedMenuItemIndex to
reference the shared findFocusedIndex (or its .toString()/string form) when
calling page.evaluate and when passing the predicate to anyPage.waitForFunction
so there is only one source of truth for locating the active
'.mdc-deprecated-list-item'.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/list-item/menu-item-meta/menu-item-meta.tsx (1)
25-63:⚠️ Potential issue | 🟠 MajorAdd documentation/example for the new
hotkeyprop.
A new prop was added to an existing component, but no usage example or explanation is shown. Please add/update the relevant component documentation (e.g., menu item docs) to demonstratehotkeyusage and behavior.As per coding guidelines,
src/components/**/*.tsx: “When contributors add new props to existing components in the lime-elements repository, they should always add documentation examples that demonstrate the new prop's usage and explain how it works.”
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/hotkey/examples/hotkey-basic.tsx`:
- Line 1: The example currently types the hotkey event payload inline as "{
hotkey: string }"; import the publicly exported type LimelHotkeyTriggerDetail
from '@limetech/lime-elements' and replace the inline type with
LimelHotkeyTriggerDetail in the event handler(s) (every place that currently
uses "{ hotkey: string }", including the handler around the hotkey trigger at
lines ~63-65). Update the top import statement to include
LimelHotkeyTriggerDetail and adjust the handler signature and any usages to
reference .value/.hotkey/.keyboardEvent from LimelHotkeyTriggerDetail for full
type safety.
In `@src/components/menu/menu.e2e.ts`:
- Around line 408-454: The test dispatches a KeyboardEvent and immediately
asserts on the spy and menu open state which can be asynchronous; after the
page.evaluate dispatch and the existing await page.waitForChanges(), add
deterministic waits for the menu to finish closing and the trigger aria state
before asserting — e.g. call waitForMenuOpenState(page, false),
waitForMenuSurfaceOpenState(page, false) and waitForTriggerAriaExpanded(page,
'button[slot="trigger"]', false) (or an equivalent waitForFunction that ensures
the spy has been called) then run the expect(spy)... and expect(isOpen)...
assertions so the test is stable.
- Around line 84-149: The wait helper's page.evaluate callbacks currently return
false when the 'limel-menu-surface' element is missing, causing a hang when
waiting for open=false; update both evaluator functions (the arrow callbacks
passed to page.evaluate that accept the isOpen parameter and query
'limel-menu-surface' into the local variable surface) so that if surface is
null/undefined you return true when isOpen is false (i.e. treat a missing
surface as closed), but keep the existing logic for the isOpen=true case; ensure
you change both the immediate wait block and the one inside the while loop so
they behave identically.
09d0d79 to
f1ae8a6
Compare
- Made it safe to import outside the browser (removed top-level `window.navigator.userAgent` access) - Added export function `isAppleDevice(): boolean`
for rendering hotkeys, and emitting events when hotkeys are pressed.
2 participants
Summary by CodeRabbit
New Features
UI
Tests
Documentation
Style
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: