Conversation
Contributor
📝 WalkthroughWalkthroughAdds display-only hotkey support to Tooltip components via a new reflected Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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-4051/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/tooltip/tooltip.tsx (1)
75-89:⚠️ Potential issue | 🟡 MinorClarify that shortcuts belong in
hotkey, nothelperLabel.The new API makes
hotkeythe dedicated shortcut field, but the adjacenthelperLabeldocs still say a keyboard shortcut is a valid example. That leaves two conflicting public guidance paths for the same feature. Please rewordhelperLabelto describe supplementary text only, and point shortcut usage tohotkey.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tooltip/tooltip.tsx` around lines 75 - 89, Update the JSDoc for the `@Prop-decorated` property helperLabel to remove the keyboard shortcut example and clarify it is for supplementary descriptive text only (e.g., explanatory notes or context for the owner element), and add a short pointer in that comment directing consumers to use the hotkey property for keyboard shortcuts; also make sure the hotkey property's JSDoc (public hotkey) remains explicit that it is the dedicated, display-only field for shortcuts.
🤖 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/tooltip/examples/tooltip-hotkey.tsx`:
- Around line 52-58: The example uses hotkey="meta+shift+s" which maps to
Win+Shift+S on Windows/Linux and mislabels the action; update the limel-tooltip
example (the limel-tooltip element with label="Save as" and hotkey prop) to use
a neutral, cross-platform shortcut (e.g., change hotkey to "ctrl+shift+s") or
alternatively adjust the label to match the platform-agnostic shortcut (e.g.,
change label from "Save as" to "Save") so the displayed hotkey and action remain
accurate across OSes.
In `@src/components/tooltip/tooltip-content.tsx`:
- Around line 53-59: The render method is returning a single JSX element wrapped
in an array; update the TooltipContent render() to return the JSX node directly
(remove the surrounding [ ... ]). Locate the render() implementation that
returns the <text class={{ 'has-column-layout': isLabelsTextLong }}
{...props}>...</text> and return that element itself rather than an array,
keeping the existing children (div.label, renderHelperLabel(), renderHotkey())
and props intact.
---
Outside diff comments:
In `@src/components/tooltip/tooltip.tsx`:
- Around line 75-89: Update the JSDoc for the `@Prop-decorated` property
helperLabel to remove the keyboard shortcut example and clarify it is for
supplementary descriptive text only (e.g., explanatory notes or context for the
owner element), and add a short pointer in that comment directing consumers to
use the hotkey property for keyboard shortcuts; also make sure the hotkey
property's JSDoc (public hotkey) remains explicit that it is the dedicated,
display-only field for shortcuts.
🪄 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: bc507023-6b27-4ba4-8792-96621c3dd000
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (6)
src/components/tooltip/examples/tooltip-hotkey.tsxsrc/components/tooltip/examples/tooltip-max-character.scsssrc/components/tooltip/tooltip-content.scsssrc/components/tooltip/tooltip-content.tsxsrc/components/tooltip/tooltip.spec.tsxsrc/components/tooltip/tooltip.tsx
Comment on lines
+52
to
+58
| <limel-button icon="save_all" id="tooltip-hotkey-2" /> | ||
| <limel-tooltip | ||
| label="Save as" | ||
| helperLabel="Save with a new name or location" | ||
| hotkey="meta+shift+s" | ||
| elementId="tooltip-hotkey-2" | ||
| /> |
Contributor
There was a problem hiding this comment.
Use a less misleading cross-platform shortcut in the “Save as” example.
meta+shift+s turns into Win+Shift+S on Windows/Linux, so this example no longer represents “Save as” there. Since this file is the docs example for the new prop, prefer a neutral shortcut or an action label that stays accurate across platforms.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/tooltip/examples/tooltip-hotkey.tsx` around lines 52 - 58, The
example uses hotkey="meta+shift+s" which maps to Win+Shift+S on Windows/Linux
and mislabels the action; update the limel-tooltip example (the limel-tooltip
element with label="Save as" and hotkey prop) to use a neutral, cross-platform
shortcut (e.g., change hotkey to "ctrl+shift+s") or alternatively adjust the
label to match the platform-agnostic shortcut (e.g., change label from "Save as"
to "Save") so the displayed hotkey and action remain accurate across OSes.
to visualize a hotkey inside the tooltip's content
…ement This honors the original intent of `label`. But screen readers will read it out load twice, since the `label` is also passed to the tooltip, and `limel-tooltip` adds it as an `aria-describedby` to the `button` element too. But it's "slightly noisy" vs "broken", and noisy wins.
No more `{...props}` spread. style is passed directly to `<Host>`,
alongside class, which makes the JSX self-documenting.
Kiarokh
force-pushed
the
hotkeys-in-more-places
branch
from
72b6303 to
c6c52e2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/components/tooltip/examples/tooltip-hotkey.tsx (1)
52-58:⚠️ Potential issue | 🟡 MinorThe “Save as” example still uses a misleading cross-platform shortcut.
meta+shift+srenders as ⊞ Win+Shift+S on Windows/Linux, where it triggers the screenshot tool rather than "Save as". This also contradicts the file's own note advising consumers to detect OS and provide differenthotkeyvalues for primary-modifier shortcuts. Considerctrl+shift+s(works on both platforms) or change the label to something neutral.♻️ Proposed fix
<limel-tooltip label="Save as" helperLabel="Save with a new name or location" - hotkey="meta+shift+s" + hotkey="ctrl+shift+s" elementId="tooltip-hotkey-2" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tooltip/examples/tooltip-hotkey.tsx` around lines 52 - 58, The example uses a platform-misleading hotkey value "meta+shift+s" on the limel-tooltip instance (paired with limel-button id "tooltip-hotkey-2"); change the hotkey prop to a cross-platform value like "ctrl+shift+s" or make the label neutral (e.g., remove the specific shortcut from label) and update the example comment/note to mention platform detection for primary-modifier shortcuts instead of showing "meta+shift+s". Ensure you edit the limel-tooltip hotkey attribute (and any displayed label text) so the example no longer suggests a shortcut that conflicts with Windows/Linux screenshot behavior.
🤖 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/tooltip/examples/tooltip-hotkey.tsx`:
- Around line 9-10: The example string "meta+c renders as <kbd>⌘</kbd>
<kbd>C</kbd> on macOS and as <kbd>⊞ Win</kbd> <kbd>+</kbd> <kbd>C</kbd> on
Windows." contains an unintended literal "+" key for the Windows rendering;
update the example in src/components/tooltip/examples/tooltip-hotkey.tsx (look
for the "meta+c renders as" text) to remove the standalone <kbd>+</kbd> so the
Windows sequence reads "<kbd>⊞ Win</kbd> <kbd>C</kbd>" matching the macOS
formatting.
In `@src/components/tooltip/examples/tooltip-max-character.scss`:
- Around line 1-6: The stylesheet filename tooltip-max-character.scss is
misleading now that it’s shared by multiple examples; rename the file to a
descriptive shared name (e.g., tooltip-examples.scss or
tooltip-example-shared.scss) and update all references to it in the example
components (tooltip-hotkey.tsx, tooltip-accessibility.tsx,
tooltip-max-character.tsx) so the styleUrl import points to the new filename,
and ensure the :host selector stays as-is in the renamed file.
In `@src/components/tooltip/tooltip-content.scss`:
- Around line 44-46: The negative margin on limel-hotkey is a magic-number hack
that couples the child to the host's padding; replace it with a clearer approach
by adding a .has-hotkey modifier on :host (or tighten :host padding when a
hotkey is present) and remove the margin-right from limel-hotkey, or at minimum
add a short explanatory comment next to limel-hotkey explaining it "undoes half
of :host padding (0.25rem) to tuck badge closer to edge" so future changes to
:host padding are obvious; update stylesheet rules that reference limel-hotkey
and :host to use the .has-hotkey modifier to keep spacing explicit and
maintainable.
In `@src/components/tooltip/tooltip-content.tsx`:
- Around line 70-81: The renderHotkey method currently drops invalid hotkey
strings silently; update renderHotkey to emit a single dev-time warning when
this.hotkey is truthy but normalizeHotkeyString(this.hotkey) returns null (i.e.,
preserve the existing behavior but call console.warn once in that branch) so
consumers see a helpful message about the invalid hotkey value; reference
renderHotkey, the hotkey prop, normalizeHotkeyString, and the limel-hotkey
output when adding the warning.
---
Duplicate comments:
In `@src/components/tooltip/examples/tooltip-hotkey.tsx`:
- Around line 52-58: The example uses a platform-misleading hotkey value
"meta+shift+s" on the limel-tooltip instance (paired with limel-button id
"tooltip-hotkey-2"); change the hotkey prop to a cross-platform value like
"ctrl+shift+s" or make the label neutral (e.g., remove the specific shortcut
from label) and update the example comment/note to mention platform detection
for primary-modifier shortcuts instead of showing "meta+shift+s". Ensure you
edit the limel-tooltip hotkey attribute (and any displayed label text) so the
example no longer suggests a shortcut that conflicts with Windows/Linux
screenshot behavior.
🪄 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: 73c93a5b-02c0-4e9a-bc8e-77e99edc5bc3
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (9)
src/components/icon-button/icon-button.tsxsrc/components/tooltip/examples/tooltip-accessibility.tsxsrc/components/tooltip/examples/tooltip-basic.tsxsrc/components/tooltip/examples/tooltip-hotkey.tsxsrc/components/tooltip/examples/tooltip-max-character.scsssrc/components/tooltip/tooltip-content.scsssrc/components/tooltip/tooltip-content.tsxsrc/components/tooltip/tooltip.spec.tsxsrc/components/tooltip/tooltip.tsx
Comment on lines
+9
to
+10
| * For example, `meta+c` renders as <kbd>⌘</kbd> <kbd>C</kbd> on macOS and | ||
| * as <kbd>⊞ Win</kbd> <kbd>+</kbd> <kbd>C</kbd> on Windows. |
Contributor
There was a problem hiding this comment.
Typo in the platform-glyph example.
The Windows rendering shows three keys (⊞ Win + + + C) instead of ⊞ Win + C. The literal + glyph between modifier and key is unintended and inconsistent with the macOS rendering on the same line. Same applies for the kbd sequence — drop the standalone <kbd>+</kbd>.
✏️ Proposed fix
- * For example, `meta+c` renders as <kbd>⌘</kbd> <kbd>C</kbd> on macOS and
- * as <kbd>⊞ Win</kbd> <kbd>+</kbd> <kbd>C</kbd> on Windows.
+ * For example, `meta+c` renders as <kbd>⌘</kbd> <kbd>C</kbd> on macOS and
+ * as <kbd>⊞ Win</kbd> <kbd>C</kbd> on Windows.📝 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.
Suggested change
| * For example, `meta+c` renders as <kbd>⌘</kbd> <kbd>C</kbd> on macOS and | |
| * as <kbd>⊞ Win</kbd> <kbd>+</kbd> <kbd>C</kbd> on Windows. | |
| * For example, `meta+c` renders as <kbd>⌘</kbd> <kbd>C</kbd> on macOS and | |
| * as <kbd>⊞ Win</kbd> <kbd>C</kbd> on Windows. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/tooltip/examples/tooltip-hotkey.tsx` around lines 9 - 10, The
example string "meta+c renders as <kbd>⌘</kbd> <kbd>C</kbd> on macOS and as
<kbd>⊞ Win</kbd> <kbd>+</kbd> <kbd>C</kbd> on Windows." contains an unintended
literal "+" key for the Windows rendering; update the example in
src/components/tooltip/examples/tooltip-hotkey.tsx (look for the "meta+c renders
as" text) to remove the standalone <kbd>+</kbd> so the Windows sequence reads
"<kbd>⊞ Win</kbd> <kbd>C</kbd>" matching the macOS formatting.
Comment on lines
+1
to
6
| :host { | ||
| display: flex; | ||
| gap: 1rem; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| justify-content: space-around; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider renaming this shared stylesheet.
This file is now referenced as styleUrl from tooltip-hotkey.tsx and tooltip-accessibility.tsx in addition to tooltip-max-character.tsx, and the host selector was generalized accordingly. The filename tooltip-max-character.scss is no longer descriptive of its scope — consider renaming it (e.g. tooltip-example-shared.scss or tooltip-examples.scss) to reflect its shared role and prevent drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/tooltip/examples/tooltip-max-character.scss` around lines 1 -
6, The stylesheet filename tooltip-max-character.scss is misleading now that
it’s shared by multiple examples; rename the file to a descriptive shared name
(e.g., tooltip-examples.scss or tooltip-example-shared.scss) and update all
references to it in the example components (tooltip-hotkey.tsx,
tooltip-accessibility.tsx, tooltip-max-character.tsx) so the styleUrl import
points to the new filename, and ensure the :host selector stays as-is in the
renamed file.
Comment on lines
+70
to
81
| private renderHotkey() { | ||
| if (!this.hotkey) { | ||
| return; | ||
| } | ||
|
|
||
| const normalized = normalizeHotkeyString(this.hotkey); | ||
| if (!normalized) { | ||
| return; | ||
| } | ||
|
|
||
| return <limel-hotkey value={normalized} />; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider warning when hotkey is provided but invalid.
When normalizeHotkeyString returns null, the component silently renders nothing for the hotkey. Since hotkey is a public prop set by consumers, a single dev-time console.warn (in this branch only) would help them notice typos like "ctrl+" rather than wondering why their shortcut never appears. Tests already cover the silent-drop behavior, so this is purely a DX improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/tooltip/tooltip-content.tsx` around lines 70 - 81, The
renderHotkey method currently drops invalid hotkey strings silently; update
renderHotkey to emit a single dev-time warning when this.hotkey is truthy but
normalizeHotkeyString(this.hotkey) returns null (i.e., preserve the existing
behavior but call console.warn once in that branch) so consumers see a helpful
message about the invalid hotkey value; reference renderHotkey, the hotkey prop,
normalizeHotkeyString, and the limel-hotkey output when adding the warning.
Kiarokh
approved these changes
Apr 27, 2026
Collaborator
|
🎉 This PR is included in version 39.17.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Accessibility
Style
Documentation
Tests
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: