Skip to content

Recommend class-based lucide icons; refactor plugin into pack-pluggable factory#635

Merged
netchampfaris merged 12 commits into
mainfrom
v1-release/icon-pack-plugin
Apr 26, 2026
Merged

Recommend class-based lucide icons; refactor plugin into pack-pluggable factory#635
netchampfaris merged 12 commits into
mainfrom
v1-release/icon-pack-plugin

Conversation

@netchampfaris

@netchampfaris netchampfaris commented Apr 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Refactor the lucide-only Tailwind plugin into a generic iconPackPlugin({ prefix, iconsDir, normalizeStrokeWidth?, defaultColor? }) factory. Lucide is now a thin wrapper; adding tabler/heroicons/etc. is a one-liner. Default export contract is preserved, so downstream tailwind configs are unaffected.
  • Add a docs page (docs/other/icons) recommending the class-based form (lucide-<name>) as the default. Documents the literal-class-name rule, how each of the three icon styles works under the hood, and component-level usage examples for Button, Dropdown, etc.
  • Migrate static lucide template usages in core components and stories from <LucideX class="..." /> to <span class="lucide-x ..." /> where it makes sense. ~icons/lucide/* imports remain available and are still recommended for genuinely dynamic / data-driven cases (KeyboardShortcut's key map, TextEditor menu config, etc.).
  • Extend SidebarItem and Tabs to detect lucide-* strings on the icon prop, matching the convention Button already uses. Lets Sidebar/Tabs configs use the recommended string form without component imports.
  • Fix three plugin-CSS regressions caught during migration:
    • display: block (matches Tailwind preflight's <svg> default) — removes phantom line-box height in parents and baseline drift next to text.
    • Drop the baked-in color: var(--ink-gray-6) so icons inherit the parent's text color, the way <svg stroke="currentColor"> does. Fixes themed buttons (e.g. theme="red") not tinting their icons.
    • Add explicit text-ink-gray-6 at internal call sites that were leaning on the previous baked-in default, to preserve the visual look there.

Test plan

  • yarn test (29/29 passing locally)
  • yarn build clean
  • yarn docs:build clean
  • Visual review: themed buttons (<Button theme=\"red\"> with icon) tint correctly
  • Visual review: icons in flex containers (Button/Toast/Sidebar/Combobox) align as before
  • Visual review: parent <div> containing only an icon collapses to icon height (no phantom line-box)
  • Verify story examples in Histoire render correctly: Sidebar/Example, Tabs/Icons, ItemList/Basic, Breadcrumbs/Slots, ListView/CustomList
  • Spot-check a downstream consumer (CRM/Gameplan) — none of the public APIs changed, but visually confirm no regressions

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • Documentation

    • New comprehensive icon documentation covering Lucide icon set integration, three usage methods (CSS utilities, component imports, auto-imported Vue components), sizing recommendations, best practices, and practical examples for common framework components.
  • Refactor

    • Icon rendering system refactored across all components to improve performance and maintainability while preserving visual appearance, user experience, and all existing functionality.

netchampfaris and others added 10 commits April 26, 2026 20:34
Pulls the lucide-specific tailwind plugin into a reusable
`iconPackPlugin({ prefix, iconsDir, normalizeStrokeWidth, defaultColor })`
factory. The lucide plugin is now a thin wrapper that preserves the
existing default export, so downstream tailwind configs are unaffected.

Opens the door to additional icon packs (tabler, heroicons, custom in-repo
packs) by registering them with their own class prefix.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Documents the three supported icon styles (class-based, ~icons/lucide
imports, auto-imported components) and recommends the class-based
form (e.g. `lucide-menu`) as the default. Calls out the literal-class-
name rule so users don't hit silent JIT misses with template-string
class names, and points the truly-dynamic case at the import form.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds short "How it works" sections under each of the three icon styles:
the Tailwind plugin's mask-image + JIT story for class-based, the Vite
virtual-module path for ~icons/lucide imports, and the unplugin-vue-
components scan that turns auto-imported tags into the same virtual
module import. Helps users reason about bundle impact and the
literal-class-name rule.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Shows the three accepted shapes (class string, component reference,
slot) on Button, and demonstrates the same convention on Dropdown
options including the dynamic-options fallback to imported components.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Swaps `<LucideX class="..." />` template-only usages for
`<span class="lucide-x ..." />` across core components: Alert, Breadcrumbs,
Combobox, ComboboxResults, Dialog, MonthPicker, MultiSelect,
MultiSelectResults, Password, Select, Sidebar, SidebarHeader,
SidebarSection, Spinner, and Toast. Drops the now-unused
~icons/lucide/* imports.

Cases that pass an icon as a value (Toast/Alert/Select/etc. accepting
user-provided icon props or rendering `<component :is="..." />` from a
data structure) are left as component refs — those are the path the
docs recommend the import form for. KeyboardShortcut and the TextEditor
modules also keep their imports for the same reason.

Behavior is identical: the class form uses the same SVG via the Tailwind
plugin's mask-image rules.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Updates story examples to demonstrate the recommended class-based icon
form (`lucide-<name>`) wherever the icon usage isn't specifically
intended to show the import-based path. Covers Alert, Breadcrumbs,
Dialog, FormControl, ItemList (Basic/CustomSlots/EmptyAndFooter/
RowStates), ListView, MultiSelect (Footer/TagsTrigger), Sidebar (Example),
Tabs (Icons), and TextInput stories.

Also extends SidebarItem and Tabs to detect `lucide-*` strings on the
`icon` prop (rendered as a span with the class) in addition to the
existing component-reference and emoji-string paths — same pattern
Button already uses. This lets Sidebar/Tabs configs use the recommended
string form without needing component imports.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds `vertical-align: -0.125em` to every generated icon class so a
mask-image icon sits on the same visual mid-line as inline text — where
the previous inline `<svg>` form rendered. Ignored in flex/grid
contexts, so icon slots in Button, Toast, Sidebar and similar are
unaffected.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Tailwind's preflight already gives every <svg> `display: block`, so the
inline-svg form the plugin replaces was block all along. Setting the
class form to inline-block was the actual deviation — it caused phantom
line-box height in parent containers and a baseline drift next to text.

Switching to `display: block` restores parity with the SVG form and
removes the need for the vertical-align nudge added in 3c3fdff. The
docs example that put a class icon inline inside a <p> is updated to
use a flex container, which is what every real usage in the codebase
already does.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The lucide plugin previously set `color: var(--ink-gray-6)` directly on
every icon class. Because that rule sits on the icon element itself it
beats any color inherited from the parent — so an icon inside a
\`Button theme=\"red\"\` (which puts text-red-700 on the button) stayed
gray instead of picking up the red.

The inline <svg> form has `stroke=\"currentColor\"` and no baked-in
default, so it inherits the parent's text color naturally. Drop the
default from the lucide wrapper to match that behavior — call sites
that want a specific tint already add `text-ink-*`.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…as relied on

Now that the lucide plugin no longer bakes in `color: var(--ink-gray-6)`,
icon spans without a `text-*` class take the parent's color. That's the
right behavior in most spots (Spinner inside themed Button, Toast close
inside white-tinted ToastClose, MemberPicker icon inside a blue pill,
etc.) but a handful of internal icons depend on the soft-gray look:
Alert dismiss X, MonthPicker calendar, Combobox/Select/MultiSelect
chevron and check indicators, plus their corresponding story examples.

Adds an explicit `text-ink-gray-6` to those call sites to preserve the
previous visual default. Skipped: spots whose parent already provides
the right color (Footer/TagsTrigger Button/Badge slots — those were the
exact case the previous fix unblocked, so we don't want to override
again).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Apr 26, 2026

Copy link
Copy Markdown

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key(s) in object: 'tools'
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
📝 Walkthrough

Walkthrough

This pull request implements a systematic migration from dynamic Vue icon component imports to CSS-class-based icon rendering across the Frappe UI component library. Lucide Vue icon components are replaced with <span> elements using corresponding lucide-* CSS classes. A new Tailwind plugin factory (iconPackPlugin) is introduced to dynamically generate icon CSS rules from SVG files, and the existing lucideIconsPlugin is refactored to use this factory. A new documentation page explaining icon usage methods is added, along with a sidebar navigation entry linking to it.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the two main changes: recommending class-based lucide icons and refactoring the plugin into a reusable factory pattern.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch v1-release/icon-pack-plugin
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch v1-release/icon-pack-plugin

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@netchampfaris netchampfaris force-pushed the v1-release/icon-pack-plugin branch from 0a77cd3 to 31d3a7f Compare April 26, 2026 15:07

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/components/Combobox/Combobox.vue (1)

557-563: ⚠️ Potential issue | 🟡 Minor

Input-mode chevron color regressed to text-ink-gray-6 (was text-ink-gray-4).

The wrapping ComboboxTrigger carries text-ink-gray-4 (line 560) which the previous <LucideChevronDown> inherited via currentColor. The new span hard-codes text-ink-gray-6, overriding the parent and darkening the chevron. Button-mode chevron at line 491 still uses text-ink-gray-4, so the two trigger modes now disagree visually.

Either drop the explicit color (let it inherit from ComboboxTrigger) or align it with the button-mode chevron.

🎨 Proposed fix
-          <span class="lucide-chevron-down size-4 text-ink-gray-6" />
+          <span class="lucide-chevron-down size-4" />
src/components/Tabs/Tabs.vue (1)

54-64: ⚠️ Potential issue | 🟡 Minor

Tab.icon type needs to accept Vue components, not just strings.

The template (lines 99–107) handles both Lucide icon strings (startsWith('lucide-')) and Vue components via <component :is="tab.icon">. The runtime guard typeof tab.icon === 'string' at line 99 only makes sense if icon can also be a component. However, both the Tab interface in types.ts and the defineSlots signatures (lines 54–64) restrict icon to string | undefined, which will mistype consumers passing component icons.

Update the Tab interface to icon?: string | Component (imported from 'vue') and ensure slot definitions match.

🧹 Nitpick comments (9)
src/components/TextInput/stories/List.vue (1)

34-34: Use size-4 for consistency with other migrated icon sites.

The rest of the migration uses size-4 (e.g., MultiSelect/stories/Footer.vue, FormControl.story.vue, Select/stories/TriggerSlots.vue). Using only w-4 here is inconsistent and risks a non-square icon if the icon class doesn't pin height (the plugin migration explicitly relies on display: block, so dimensions come from utilities).

As per coding guidelines: "Use size-* Tailwind utility for square elements".

♻️ Proposed change
-          <span class="lucide-search w-4 text-ink-gray-6" />
+          <span class="lucide-search size-4 text-ink-gray-6" />

Also applies to: 42-42

src/components/Sidebar/SidebarHeader.vue (1)

54-54: Prefer size-4 over h-4 w-4.

As per coding guidelines: "Use size-* Tailwind utility for square elements".

♻️ Proposed change
-          <span class="lucide-chevron-down h-4 w-4 text-ink-gray-7" />
+          <span class="lucide-chevron-down size-4 text-ink-gray-7" />
src/components/Dialog/stories/Interactive.vue (1)

29-29: Use semantic ink token and size-* utility.

Other migrated icon spans in this PR (and in this very Dialog) use size-4 text-ink-gray-*. text-gray-500 is a raw Tailwind color, and h-4 w-4 can be collapsed to size-4 per repo conventions.

♻️ Suggested change
-              <span class="lucide-chevron-down h-4 w-4 text-gray-500" />
+              <span class="lucide-chevron-down size-4 text-ink-gray-5" />

As per coding guidelines: "Use semantic design tokens over hardcoded colors: bg-surface-*, text-ink-*, …" and "Use size-* Tailwind utility for square elements".

src/components/Dialog/Dialog.vue (1)

71-71: Optional: collapse h-4 w-4 to size-4.

Repo convention prefers size-* for square elements; the rest of the migrated icon spans in this PR already use size-4.

♻️ Suggested change
-                                <span class="lucide-x h-4 w-4 text-ink-gray-9" />
+                                <span class="lucide-x size-4 text-ink-gray-9" />

As per coding guidelines: "Use size-* Tailwind utility for square elements".

src/components/Sidebar/SidebarItem.vue (1)

26-41: Optional: hoist the icon-shape branching into a computed.

The two props.icon && typeof props.icon === 'string' guards plus the startsWith('lucide-') check are duplicated and a bit noisy in the template. A small computed (e.g. iconKind: 'lucide-class' | 'text' | 'component' | 'none') would mirror the pattern Select/MultiSelect use via isLucideIconString / isEmojiIconString and keep the template flat.

Functionally the change is correct and backwards-compatible.

src/components/Sidebar/SidebarSection.vue (1)

20-24: Use size-4 instead of w-4 h-4.

Square element — coding guideline prefers the size-* utility.

♻️ Proposed tweak
-        <span
-          v-if="!isSidebarCollapsed"
-          class="lucide-chevron-right w-4 h-4 text-ink-gray-5 transition-all duration-300 ease-in-out"
-          :class="{ 'rotate-90': !isCollapsed }"
-        />
+        <span
+          v-if="!isSidebarCollapsed"
+          class="lucide-chevron-right size-4 text-ink-gray-5 transition-all duration-300 ease-in-out"
+          :class="{ 'rotate-90': !isCollapsed }"
+        />

As per coding guidelines: "Use size-* Tailwind utility for square elements".

src/components/Tabs/Tabs.vue (1)

98-107: Consider also handling emoji/legacy string icons for parity with selection components.

MultiSelectResults/Combobox route icon strings through isLucideIconString and isEmojiIconString (see src/utils/iconString.ts). Tabs only treats lucide--prefixed strings as icons; any other string falls into <component :is="tab.icon">, which Vue will try to resolve as a globally registered component name and silently render nothing if missing. If Tabs is meant to accept the same icon vocabulary as selection menus, reuse the helper.

tailwind/iconPackPlugin.js (2)

5-23: JSDoc says "inline-block" but the rule emits display: block.

The header doc on Line 7 describes each generated class as rendering as "an inline-block square", but the actual rule on Line 86 sets display: block (intentionally, per the inline comment on Lines 82–85, and consistent with what docs/content/docs/other/icons.md Line 56 tells consumers). Worth aligning the JSDoc so the public docstring matches behavior.

📝 Proposed JSDoc tweak
 /**
  * Builds a Tailwind plugin that exposes every SVG in `iconsDir` as a
- * `<prefix>-<name>` utility class. Each class renders as an inline-block
- * square that masks the icon SVG with the current text color — size with
- * `size-*`, tint with `text-*`:
+ * `<prefix>-<name>` utility class. Each class renders as a block-level
+ * square (matching Tailwind preflight's `svg { display: block }`) that
+ * masks the icon SVG with the current text color — size with `size-*`,
+ * tint with `text-*`:
  *
  *   <span class="lucide-menu size-4 text-ink-gray-6" />

45-50: stroke-width regex lacks the /g flag — only the first occurrence is normalized.

Lucide-static icons happen to declare stroke-width only on the root <svg>, so this is fine for the lucide wrapper today. But the factory is explicitly documented as reusable for "any pack that ships a flat directory of named SVGs" (Lines 13–14), and packs like Tabler/Heroicons/custom packs sometimes set stroke-width on inner elements. Adding /g keeps normalizeStrokeWidth honest for those cases at zero cost.

♻️ Proposed fix
     if (normalizeStrokeWidth != null) {
       svg = svg.replace(
-        /stroke-width="[^"]+"/,
+        /stroke-width="[^"]+"/g,
         `stroke-width="${normalizeStrokeWidth}"`,
       )
     }

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ac3d2102-bb9b-4260-ab3b-b6db00233ae1

📥 Commits

Reviewing files that changed from the base of the PR and between c011a6d and 0a77cd3.

📒 Files selected for processing (36)
  • docs/components/Docs/Sidebar.vue
  • docs/content/docs/other/icons.md
  • src/components/Alert/Alert.vue
  • src/components/Alert/stories/Slots.vue
  • src/components/Breadcrumbs/Breadcrumbs.vue
  • src/components/Breadcrumbs/stories/Slots.vue
  • src/components/Combobox/Combobox.vue
  • src/components/Combobox/ComboboxResults.vue
  • src/components/Dialog/Dialog.vue
  • src/components/Dialog/stories/Interactive.vue
  • src/components/FormControl/FormControl.story.vue
  • src/components/ItemList/stories/Basic.vue
  • src/components/ItemList/stories/CustomSlots.vue
  • src/components/ItemList/stories/EmptyAndFooter.vue
  • src/components/ItemList/stories/RowStates.vue
  • src/components/ListView/stories/CustomList.vue
  • src/components/MonthPicker/MonthPicker.vue
  • src/components/MultiSelect/MultiSelect.vue
  • src/components/MultiSelect/MultiSelectResults.vue
  • src/components/MultiSelect/stories/Footer.vue
  • src/components/MultiSelect/stories/TagsTrigger.vue
  • src/components/Password/Password.vue
  • src/components/Select/Select.vue
  • src/components/Select/stories/TriggerSlots.vue
  • src/components/Sidebar/Sidebar.vue
  • src/components/Sidebar/SidebarHeader.vue
  • src/components/Sidebar/SidebarItem.vue
  • src/components/Sidebar/SidebarSection.vue
  • src/components/Sidebar/stories/Example.vue
  • src/components/Spinner.vue
  • src/components/Tabs/Tabs.vue
  • src/components/Tabs/stories/Icons.vue
  • src/components/TextInput/stories/List.vue
  • src/components/Toast/Toast.vue
  • tailwind/iconPackPlugin.js
  • tailwind/lucideIconsPlugin.js

Comment thread src/components/Alert/Alert.vue Outdated
Comment on lines 81 to 83
<button v-if="props.dismissable" @click="dismissAlert">
<LucideX class="size-4" />
<span class="lucide-x size-4 text-ink-gray-6" />
</button>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add an accessible name to the dismiss button.

The dismiss button now contains only a decorative <span> (no SVG <title>, no text node), so screen readers expose it without a name. Worth adding aria-label="Dismiss" while you're touching this code path.

🛡️ Proposed fix
-    <button v-if="props.dismissable" `@click`="dismissAlert">
-      <span class="lucide-x size-4 text-ink-gray-6" />
-    </button>
+    <button
+      v-if="props.dismissable"
+      type="button"
+      aria-label="Dismiss"
+      `@click`="dismissAlert"
+    >
+      <span class="lucide-x size-4 text-ink-gray-6" aria-hidden="true" />
+    </button>
📝 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
<button v-if="props.dismissable" @click="dismissAlert">
<LucideX class="size-4" />
<span class="lucide-x size-4 text-ink-gray-6" />
</button>
<button
v-if="props.dismissable"
type="button"
aria-label="Dismiss"
`@click`="dismissAlert"
>
<span class="lucide-x size-4 text-ink-gray-6" aria-hidden="true" />
</button>

<Button variant="ghost">
<template #icon>
<LucideEllipsis class="w-4 text-ink-gray-5" />
<span class="lucide-ellipsis w-4 text-ink-gray-5" />

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use size-4 instead of w-4 for a square icon.

The iconPackPlugin sets width: 1em; height: 1em on .lucide-*. w-4 overrides only the width, leaving the height tied to the parent font-size (1em). The previous <LucideEllipsis> SVG was intrinsically square, so w-4 was sufficient; with the class-based form you need to constrain both dimensions to keep it square across font sizes.

♻️ Suggested fix
-            <span class="lucide-ellipsis w-4 text-ink-gray-5" />
+            <span class="lucide-ellipsis size-4 text-ink-gray-5" />
📝 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
<span class="lucide-ellipsis w-4 text-ink-gray-5" />
<span class="lucide-ellipsis size-4 text-ink-gray-5" />

Comment on lines +29 to 34
<span
v-show="showEye"
:is="show ? LucideEyeOff : LucideEye"
:class="show ? 'lucide-eye-off' : 'lucide-eye'"
class="h-3 cursor-pointer mr-1"
@click="show = !show"
/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Icon will render non-square — switch h-3 to size-3.

The class-based Lucide icon uses width: 1em; height: 1em from the plugin. Keeping only h-3 leaves the width at 1em (≈16px in a text-base context) while compressing the height to 12px, producing a stretched/non-square icon. The original <LucideEye> SVG was intrinsically square, which is why h-3 alone worked before.

♻️ Suggested fix
-          <span
-            v-show="showEye"
-            :class="show ? 'lucide-eye-off' : 'lucide-eye'"
-            class="h-3 cursor-pointer mr-1"
-            `@click`="show = !show"
-          />
+          <span
+            v-show="showEye"
+            :class="show ? 'lucide-eye-off' : 'lucide-eye'"
+            class="size-3 cursor-pointer mr-1"
+            `@click`="show = !show"
+          />
📝 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
<span
v-show="showEye"
:is="show ? LucideEyeOff : LucideEye"
:class="show ? 'lucide-eye-off' : 'lucide-eye'"
class="h-3 cursor-pointer mr-1"
@click="show = !show"
/>
<span
v-show="showEye"
:class="show ? 'lucide-eye-off' : 'lucide-eye'"
class="size-3 cursor-pointer mr-1"
`@click`="show = !show"
/>

Comment on lines +21 to 24
<span
v-else-if="type == 'error'"
class="flex-shrink-0 size-4 text-ink-red-2"
class="lucide-info flex-shrink-0 size-4 text-ink-red-2"
/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether the error→info mapping pre-existed or was introduced in this PR.
git log --all --oneline -- src/components/Toast/Toast.vue | head -20
git log -p --all -- src/components/Toast/Toast.vue | rg -n -C2 "type == 'error'|LucideInfo|lucide-info" | head -80

Repository: frappe/frappe-ui

Length of output: 311


🏁 Script executed:

#!/bin/bash
# Get the full current state of Toast.vue to see all icon mappings
cat -n src/components/Toast/Toast.vue | head -60

Repository: frappe/frappe-ui

Length of output: 2594


🏁 Script executed:

#!/bin/bash
# Get the git diff to see what changed in this specific file
git diff HEAD -- src/components/Toast/Toast.vue | head -100

Repository: frappe/frappe-ui

Length of output: 42


🏁 Script executed:

#!/bin/bash
# Search for available Lucide error-related icons
rg "lucide-(circle-alert|octagon|alert|x-circle|alert-circle)" --max-count=20

Repository: frappe/frappe-ui

Length of output: 169


🏁 Script executed:

#!/bin/bash
# Check git log to see if Toast.vue was recently created or modified
git log --follow --pretty=format:"%h %s" -- src/components/Toast/Toast.vue

Repository: frappe/frappe-ui

Length of output: 149


🏁 Script executed:

#!/bin/bash
# Search for all lucide icons available in the codebase to identify appropriate error icons
rg "lucide-[a-z-]+" -o | sort | uniq

Repository: frappe/frappe-ui

Length of output: 8162


🏁 Script executed:

#!/bin/bash
# Check the git show for the most recent commit that touched Toast.vue to see what changed
git show HEAD:src/components/Toast/Toast.vue 2>/dev/null | head -40

Repository: frappe/frappe-ui

Length of output: 1519


Error toast renders an info icon — semantically incorrect.

type === 'error' renders lucide-info, which visually communicates "information" rather than "error" and conflicts with accessibility expectations. The Alert component in the same codebase uses lucide-circle-x for error states; align Toast with that convention.

🛠 Proposed fix
        <span
          v-else-if="type == 'error'"
-         class="lucide-info flex-shrink-0 size-4 text-ink-red-2"
+         class="lucide-circle-x flex-shrink-0 size-4 text-ink-red-2"
        />
📝 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
<span
v-else-if="type == 'error'"
class="flex-shrink-0 size-4 text-ink-red-2"
class="lucide-info flex-shrink-0 size-4 text-ink-red-2"
/>
<span
v-else-if="type == 'error'"
class="lucide-circle-x flex-shrink-0 size-4 text-ink-red-2"
/>

@github-actions

github-actions Bot commented Apr 26, 2026

Copy link
Copy Markdown
Contributor

🚀 VitePress preview is ready:

https://ui.frappe.io/pr-preview/pr-635/

github-actions Bot added a commit that referenced this pull request Apr 26, 2026
- Alert: add aria-label="Dismiss" to dismiss button, aria-hidden on icon
- Combobox: drop hardcoded color on input-mode chevron so it inherits parent
- Tabs: widen Tab.icon type to string | Component to match template
- Normalize h-4 w-4 / w-4 / h-3 to size-* across migrated icon spans
- Dialog story: swap raw text-gray-500 for semantic text-ink-gray-5
- iconPackPlugin: align JSDoc with display:block; /g flag on stroke-width regex

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
github-actions Bot added a commit that referenced this pull request Apr 26, 2026
KeyboardShortcut's key→icon map now stores lucide-* class strings instead
of imported components; the data-driven shape stays the same. TocNodeView's
single static <LucideX> becomes a class-based span.

Drops 9 ~icons/lucide imports from the bundle entry path.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
github-actions Bot added a commit that referenced this pull request Apr 26, 2026
@netchampfaris netchampfaris merged commit 317744e into main Apr 26, 2026
7 checks passed
github-actions Bot added a commit that referenced this pull request Apr 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant