feat(icons): allow 3rd party packages to provide icon names for auto completion#3778
feat(icons): allow 3rd party packages to provide icon names for auto completion#3778
Conversation
📝 WalkthroughWalkthroughIntroduced an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
jgroth
force-pushed
the
autocomplete-icons
branch
from
b4a0507 to
0f3ea9d
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
🤖 Fix all issues with AI agents
In `@src/global/shared-types/icon.types.ts`:
- Around line 3-21: Add a Biome ignore comment immediately above the
intentionally empty IconNameRegistry interface to suppress the noEmptyInterface
rule; place a comment like /* biome-ignore no-empty-interface */ directly before
the export interface IconNameRegistry {} declaration so the linter ignores this
specific rule for that empty interface used for module augmentation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
src/components/action-bar/action-bar.types.tssrc/components/banner/banner.tsxsrc/components/button/button.tsxsrc/components/chip-set/chip-set.tsxsrc/components/dock/dock.types.tssrc/components/icon-button/icon-button.tsxsrc/components/input-field/input-field.tsxsrc/components/picker/picker.tsxsrc/components/profile-picture/profile-picture.tsxsrc/components/shortcut/shortcut.tsxsrc/components/split-button/split-button.tsxsrc/global/shared-types/icon.types.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/wrap-multiple-jsx-elements-in-host-not-in-array.mdc)
When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element. When there is already a single top-level element, it does not have to be wrapped in<Host>.
Files:
src/components/input-field/input-field.tsxsrc/components/chip-set/chip-set.tsxsrc/components/banner/banner.tsxsrc/components/profile-picture/profile-picture.tsxsrc/components/picker/picker.tsxsrc/components/shortcut/shortcut.tsxsrc/components/split-button/split-button.tsxsrc/components/icon-button/icon-button.tsxsrc/components/button/button.tsx
⚙️ CodeRabbit configuration file
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
Files:
src/components/input-field/input-field.tsxsrc/components/chip-set/chip-set.tsxsrc/components/banner/banner.tsxsrc/components/profile-picture/profile-picture.tsxsrc/components/picker/picker.tsxsrc/components/shortcut/shortcut.tsxsrc/components/split-button/split-button.tsxsrc/components/icon-button/icon-button.tsxsrc/components/button/button.tsx
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
Files:
src/components/input-field/input-field.tsxsrc/components/chip-set/chip-set.tsxsrc/components/banner/banner.tsxsrc/components/dock/dock.types.tssrc/components/profile-picture/profile-picture.tsxsrc/components/picker/picker.tsxsrc/components/shortcut/shortcut.tsxsrc/global/shared-types/icon.types.tssrc/components/split-button/split-button.tsxsrc/components/icon-button/icon-button.tsxsrc/components/action-bar/action-bar.types.tssrc/components/button/button.tsx
**/*.{tsx,scss}
⚙️ CodeRabbit configuration file
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
Files:
src/components/input-field/input-field.tsxsrc/components/chip-set/chip-set.tsxsrc/components/banner/banner.tsxsrc/components/profile-picture/profile-picture.tsxsrc/components/picker/picker.tsxsrc/components/shortcut/shortcut.tsxsrc/components/split-button/split-button.tsxsrc/components/icon-button/icon-button.tsxsrc/components/button/button.tsx
src/components/**/*.tsx
⚙️ CodeRabbit configuration file
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. This helps with user adoption, feature discoverability, and maintains comprehensive documentation.
Files:
src/components/input-field/input-field.tsxsrc/components/chip-set/chip-set.tsxsrc/components/banner/banner.tsxsrc/components/profile-picture/profile-picture.tsxsrc/components/picker/picker.tsxsrc/components/shortcut/shortcut.tsxsrc/components/split-button/split-button.tsxsrc/components/icon-button/icon-button.tsxsrc/components/button/button.tsx
🧠 Learnings (4)
📓 Common learnings
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.
📚 Learning: 2025-04-25T13:32:56.396Z
Learnt from: adrianschmidt
Repo: Lundalogik/lime-elements PR: 3529
File: src/components/lime-ai-avatar/lime-ai-avatar.tsx:30-38
Timestamp: 2025-04-25T13:32:56.396Z
Learning: For StencilJS components in lime-elements, prefer using the `<Host>` element to wrap child elements in the render method instead of returning an array of elements, which eliminates the need for key attributes and follows StencilJS best practices.
Applied to files:
src/components/banner/banner.tsx
📚 Learning: 2025-04-25T13:32:56.396Z
Learnt from: adrianschmidt
Repo: Lundalogik/lime-elements PR: 3529
File: src/components/lime-ai-avatar/lime-ai-avatar.tsx:30-38
Timestamp: 2025-04-25T13:32:56.396Z
Learning: For StencilJS components in lime-elements, prefer using the `<Host>` element to wrap child elements in the render method instead of returning an array of elements. This follows the standard pattern in the codebase and eliminates the need for key attributes.
Applied to files:
src/components/banner/banner.tsx
📚 Learning: 2025-04-17T09:39:36.254Z
Learnt from: adrianschmidt
Repo: Lundalogik/lime-elements PR: 3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using 'limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
Applied to files:
src/components/profile-picture/profile-picture.tsxsrc/components/picker/picker.tsxsrc/components/shortcut/shortcut.tsxsrc/global/shared-types/icon.types.ts
🧬 Code graph analysis (11)
src/components/input-field/input-field.tsx (1)
src/global/shared-types/icon.types.ts (1)
IconName(31-31)
src/components/chip-set/chip-set.tsx (1)
src/global/shared-types/icon.types.ts (1)
IconName(31-31)
src/components/banner/banner.tsx (1)
src/global/shared-types/icon.types.ts (1)
IconName(31-31)
src/components/dock/dock.types.ts (1)
src/global/shared-types/icon.types.ts (1)
IconName(31-31)
src/components/profile-picture/profile-picture.tsx (1)
src/global/shared-types/icon.types.ts (2)
IconName(31-31)Icon(38-68)
src/components/picker/picker.tsx (1)
src/global/shared-types/icon.types.ts (1)
IconName(31-31)
src/components/shortcut/shortcut.tsx (1)
src/global/shared-types/icon.types.ts (1)
IconName(31-31)
src/components/split-button/split-button.tsx (1)
src/global/shared-types/icon.types.ts (1)
IconName(31-31)
src/components/icon-button/icon-button.tsx (1)
src/global/shared-types/icon.types.ts (2)
IconName(31-31)Icon(38-68)
src/components/action-bar/action-bar.types.ts (1)
src/global/shared-types/icon.types.ts (2)
IconName(31-31)Icon(38-68)
src/components/button/button.tsx (1)
src/global/shared-types/icon.types.ts (2)
IconName(31-31)Icon(38-68)
🪛 Biome (2.1.2)
src/global/shared-types/icon.types.ts
[error] 21-21: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
🔇 Additional comments (12)
src/components/action-bar/action-bar.types.ts (1)
1-19: LGTM — IconName integration is consistent.Line 19 aligns the icon typing with the new IconName registry without changing runtime behavior.
src/global/shared-types/icon.types.ts (1)
38-42: LGTM — Icon.name now uses IconName.Line 42 correctly aligns Icon with the new IconName type.
src/components/dock/dock.types.ts (1)
1-20: LGTM — DockItem.icon typed as IconName.Line 20 aligns with the new IconName registry and preserves behavior.
src/components/icon-button/icon-button.tsx (1)
7-28: LGTM — Icon prop now supports IconName | Icon.Line 28 correctly tightens typing while retaining compatibility.
src/components/banner/banner.tsx (1)
2-24: LGTM — Banner icon now typed as IconName.Line 24 aligns with the shared IconName type without changing behavior.
src/components/shortcut/shortcut.tsx (1)
5-32: Icon prop typing alignment looks good.Switching to
IconNamekeeps the runtime behavior intact while aligning the public API with the shared icon typing.src/components/chip-set/chip-set.tsx (1)
21-21: Leading icon typing update is consistent.Using
IconNameforleadingIconmatches the shared icon types and should improve autocomplete without behavioral changes.Also applies to: 174-174
src/components/button/button.tsx (1)
6-50: Button icon typing update looks correct.
IconName | Iconkeeps existing behavior and aligns the public API with the shared icon types.src/components/split-button/split-button.tsx (1)
4-45: SplitButton icon typing change is consistent.
IconNameusage matches the shared icon typing strategy without changing rendering behavior.src/components/profile-picture/profile-picture.tsx (1)
13-63: ProfilePicture icon typing update looks good.Switching to
IconName | Iconkeeps behavior intact while aligning with the shared icon typing.src/components/picker/picker.tsx (1)
26-26: IconName alignment looks good.This keeps the prop consistent with the shared icon typing for autocomplete and broader icon registry usage.
Also applies to: 85-85
src/components/input-field/input-field.tsx (1)
28-28: IconName prop typing is consistent and appropriate.Aligns the input field with the centralized icon type for safer autocomplete.
Also applies to: 135-135, 141-141
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
jgroth
force-pushed
the
autocomplete-icons
branch
from
0f3ea9d to
b795194
Compare
jgroth
force-pushed
the
autocomplete-icons
branch
2 times, most recently
from
74751e3 to
b6f4a28
Compare
jgroth
force-pushed
the
autocomplete-icons
branch
from
b6f4a28 to
d6b98cd
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/button/button.tsx (1)
152-152: 🧹 Nitpick | 🔵 TrivialUpdate
renderIconparameter type to match the new prop type.The private method signature still uses
string | Icon, but theiconprop is nowIconName | Icon. This should be aligned for consistency.Proposed fix
- private renderIcon(icon?: string | Icon) { + private renderIcon(icon?: IconName | Icon) {src/components/input-field/input-field.tsx (1)
761-761: 🧹 Nitpick | 🔵 TrivialConsider updating private helper signatures to use
IconNamefor consistency.
renderLinkIconandrenderTrailingIconaccepticon: string, but they now receive values originating fromIconName-typed properties. While this is type-safe (sinceIconNameis assignable tostring), updating these private signatures toIconNamewould keep the typing consistent throughout the file.Suggested diff
- private renderLinkIcon = (linkProps: LinkProperties, icon: string) => { + private renderLinkIcon = (linkProps: LinkProperties, icon: IconName) => {- private renderTrailingIcon = (icon: string) => { + private renderTrailingIcon = (icon: IconName) => {Also applies to: 779-779
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3778/ |
There was a problem hiding this comment.
Just making CodeRabbit's "outside the diff" comments more visible 🙂
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)src/components/button/button.tsx (1)>
152-152: 🧹 Nitpick | 🔵 TrivialUpdate
renderIconparameter type to match the new prop type.
The private method signature still usesstring | Icon, but theiconprop is nowIconName | Icon. This should be aligned for consistency.Proposed fix
- private renderIcon(icon?: string | Icon) { + private renderIcon(icon?: IconName | Icon) {src/components/input-field/input-field.tsx (1)>
761-761: 🧹 Nitpick | 🔵 TrivialConsider updating private helper signatures to use
IconNamefor consistency.
renderLinkIconandrenderTrailingIconaccepticon: string, but they now receive values originating fromIconName-typed properties. While this is type-safe (sinceIconNameis assignable tostring), updating these private signatures toIconNamewould keep the typing consistent throughout the file.Suggested diff
- private renderLinkIcon = (linkProps: LinkProperties, icon: string) => { + private renderLinkIcon = (linkProps: LinkProperties, icon: IconName) => {- private renderTrailingIcon = (icon: string) => { + private renderTrailingIcon = (icon: IconName) => {Also applies to: 779-779
…r auto completion
…r auto completion
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/dynamic-label/label.types.ts`:
- Line 1: The import in label.types.ts is inconsistent: replace the current
import of IconName and Icon from '../../interface' with the same module used
elsewhere ('../../global/shared-types/icon.types') so IconName and Icon are
imported from the direct shared types module; update the import statement that
references IconName and Icon to point to '../../global/shared-types/icon.types'
to match other files (stay mindful of the symbols IconName and Icon and the file
label.types.ts when editing).
In `@src/global/shared-types/file.types.ts`:
- Line 2: The import for Icon and IconName uses an overly verbose path; update
the import in file.types.ts to use the local sibling module path by replacing
the '../../global/shared-types/icon.types' import with './icon.types' so it
matches the other same-directory imports and preserves the Icon and IconName
symbols.
| @@ -1,4 +1,4 @@ | |||
| import { Icon } from '../../interface'; | |||
| import { IconName, Icon } from '../../interface'; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent import path compared to other files in this PR.
All other files in this PR import IconName and Icon from '../../global/shared-types/icon.types', but this file imports from '../../interface'. While both are valid relative paths, consider aligning to the direct module path for consistency.
🤖 Prompt for AI Agents
In `@src/components/dynamic-label/label.types.ts` at line 1, The import in
label.types.ts is inconsistent: replace the current import of IconName and Icon
from '../../interface' with the same module used elsewhere
('../../global/shared-types/icon.types') so IconName and Icon are imported from
the direct shared types module; update the import statement that references
IconName and Icon to point to '../../global/shared-types/icon.types' to match
other files (stay mindful of the symbols IconName and Icon and the file
label.types.ts when editing).
| @@ -1,5 +1,5 @@ | |||
| import { MenuItem } from '../../components'; | |||
| import { Icon } from '../../global/shared-types/icon.types'; | |||
| import { Icon, IconName } from '../../global/shared-types/icon.types'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the actual files
echo "=== Finding file.types.ts ==="
find . -name 'file.types.ts' -type f
echo "=== Finding icon.types.ts ==="
find . -name 'icon.types.ts' -type f
echo "=== Checking directory structure around shared-types ==="
fd --type d 'shared-types' | head -5Repository: Lundalogik/lime-elements
Length of output: 325
🏁 Script executed:
#!/bin/bash
# Read the file.types.ts to see the actual import
echo "=== Content of src/global/shared-types/file.types.ts (first 20 lines) ==="
cat -n src/global/shared-types/file.types.ts | head -20Repository: Lundalogik/lime-elements
Length of output: 684
🏁 Script executed:
#!/bin/bash
# Check if other files in the same directory use './icon.types' or the longer path
echo "=== Checking other imports of icon.types in the same directory ==="
rg "from.*icon\.types" src/global/shared-types/ -A 0 -B 0Repository: Lundalogik/lime-elements
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Check all imports within shared-types files to see the pattern used
echo "=== All imports in files within src/global/shared-types/ ==="
rg "^import.*from" src/global/shared-types/*.ts | head -30Repository: Lundalogik/lime-elements
Length of output: 563
Simplify the relative import path for consistency.
This file uses simplified relative imports (./color.types, ./separator.types) for other same-directory imports, but uses an unnecessarily verbose path for icon.types. Since both files are in src/global/shared-types/, use ./icon.types instead of ../../global/shared-types/icon.types to match the pattern used elsewhere in the file.
Proposed fix
-import { Icon, IconName } from '../../global/shared-types/icon.types';
+import { Icon, IconName } from './icon.types';📝 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.
| import { Icon, IconName } from '../../global/shared-types/icon.types'; | |
| import { Icon, IconName } from './icon.types'; |
🤖 Prompt for AI Agents
In `@src/global/shared-types/file.types.ts` at line 2, The import for Icon and
IconName uses an overly verbose path; update the import in file.types.ts to use
the local sibling module path by replacing the
'../../global/shared-types/icon.types' import with './icon.types' so it matches
the other same-directory imports and preserves the Icon and IconName symbols.
|
@jgroth, do you want me to ask @adrianschmidt-bot to finish this one for you? |
|
I went through the review comments and prepared fixes for all of them. However, since this branch lives on the upstream repo (not our fork), I don't have push access. Here's what I've prepared: Fixes addressing all 4 review comments:
Full diff (apply with
|
3 participants
fix Lundalogik/crm-client#622
Summary by CodeRabbit
How to test
@limetech/lime-elements@limetech/lime-elementsinside@lundalogik/lime-icons8@lundalogik/lime-icons8@limetech/lime-elementsand@lundalogik/lime-icons8inside any package (or test in https://github.com/Lundalogik/lime-crm-building-blocks/pull/1264 or https://github.com/Lundalogik/lime-crm-components/pull/3994)import '@lundalogik/lime-icons8';somewhereReview:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: