Skip to content

feat: tree-shakeable string icons <Button icon="menu" /> #627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
netchampfaris wants to merge 1 commit into
base: main
Choose a base branch
from
+424 −94
Open

feat: tree-shakeable string icons <Button icon="menu" /> #627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/Button/Button.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe('<Button />', () => {
cy.get('svg').should('exist') // Loading Spinner
})

it('handles prefix and suffix slots (replacing deprecated icon props)', () => {
it('handles prefix and suffix slots', () => {
const TestIcon = {
render() {
return h('svg', { 'data-cy': 'test-icon' })
Expand Down
129 changes: 77 additions & 52 deletions src/components/Button/Button.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
<Tooltip :text="tooltip" :disabled="!tooltip?.length">
<button
v-bind="$attrs"
ref="rootRef"
:class="buttonClasses"
@click="handleClick"
:disabled="isDisabled"
:ariaLabel="label"
:type = "props.type"
ref="rootRef"
:aria-label="label"
:type="props.type"
@click="handleClick"
>
<LoadingIndicator
v-if="loading"
Expand All @@ -18,24 +18,22 @@
'h-4.5 w-4.5': size == 'xl' || size == '2xl',
}"
/>
<slot name="prefix" v-else-if="$slots['prefix'] || iconLeft">
<FeatherIcon
v-if="iconLeft && typeof iconLeft === 'string'"
:name="iconLeft"
<slot name="prefix" v-else-if="$slots.prefix || iconLeftComponent">
<component
v-if="iconLeftComponent"
:is="iconLeftComponent"
:class="slotClasses"
aria-hidden="true"
/>
<component v-else-if="iconLeft" :is="iconLeft" :class="slotClasses" />
</slot>

<template v-if="loading && loadingText">{{ loadingText }}</template>
<template v-else-if="isIconButton && !loading">
<FeatherIcon
v-if="icon && typeof icon === 'string'"
:name="icon"
<component
v-if="iconComponent"
:is="iconComponent"
:class="slotClasses"
/>
<component v-else-if="icon" :is="icon" :class="slotClasses" />
<slot name="icon" v-else-if="$slots.icon" />
<div v-else-if="hasLucideIconInDefaultSlot" :class="slotClasses">
<slot>{{ label }}</slot>
Expand All @@ -46,28 +44,25 @@
</span>

<slot name="suffix">
<FeatherIcon
v-if="iconRight && typeof iconRight === 'string'"
:name="iconRight"
<component
v-if="iconRightComponent"
:is="iconRightComponent"
:class="slotClasses"
aria-hidden="true"
/>
<component
v-else-if="iconRight"
:is="iconRight"
:class="slotClasses"
/>
</slot>
</button>
</Tooltip>
</template>

<script lang="ts" setup>
import { computed, useSlots, ref } from 'vue'
import FeatherIcon from '../FeatherIcon.vue'
import LoadingIndicator from '../LoadingIndicator.vue'
import { computed, ref, useSlots, watchEffect } from 'vue'
import { useRouter } from 'vue-router'
import type { ButtonProps, ThemeVariant } from './types'
import LoadingIndicator from '../LoadingIndicator.vue'
import Tooltip from '../Tooltip/Tooltip.vue'
import type { ButtonProps, ThemeVariant } from './types'

const warnedRuntimeStringIcons = new Set<string>()

defineOptions({ inheritAttrs: false })

Expand All @@ -77,62 +72,62 @@ const props = withDefaults(defineProps<ButtonProps>(), {
variant: 'subtle',
loading: false,
disabled: false,
type: "button"
type: 'button',
})

const slots = useSlots()
const router = useRouter()

const buttonClasses = computed(() => {
let solidClasses = {
const solidClasses = {
gray: 'text-ink-white bg-surface-gray-7 hover:bg-surface-gray-6 active:bg-surface-gray-5',
blue: 'text-ink-white bg-blue-500 hover:bg-surface-blue-3 active:bg-blue-700',
green:
'text-ink-white bg-surface-green-3 hover:bg-green-700 active:bg-green-800',
red: 'text-ink-white bg-surface-red-5 hover:bg-surface-red-6 active:bg-surface-red-7',
}[props.theme]

let subtleClasses = {
const subtleClasses = {
gray: 'text-ink-gray-8 bg-surface-gray-2 hover:bg-surface-gray-3 active:bg-surface-gray-4',
blue: 'text-ink-blue-3 bg-surface-blue-2 hover:bg-blue-200 active:bg-blue-300',
green:
'text-green-800 bg-surface-green-2 hover:bg-green-200 active:bg-green-300',
red: 'text-red-700 bg-surface-red-2 hover:bg-surface-red-3 active:bg-surface-red-4',
}[props.theme]

let outlineClasses = {
const outlineClasses = {
gray: 'text-ink-gray-8 bg-surface-white bg-surface-white border border-outline-gray-2 hover:border-outline-gray-3 active:border-outline-gray-3 active:bg-surface-gray-4',
blue: 'text-ink-blue-3 bg-surface-white border border-outline-blue-1 hover:border-blue-400 active:border-blue-400 active:bg-blue-300',
green:
'text-green-800 bg-surface-white border border-outline-green-2 hover:border-green-500 active:border-green-500 active:bg-green-300',
red: 'text-red-700 bg-surface-white border border-outline-red-1 hover:border-outline-red-2 active:border-outline-red-2 active:bg-surface-red-3',
}[props.theme]

let ghostClasses = {
const ghostClasses = {
gray: 'text-ink-gray-8 bg-transparent hover:bg-surface-gray-3 active:bg-surface-gray-4',
blue: 'text-ink-blue-3 bg-transparent hover:bg-blue-200 active:bg-blue-300',
green:
'text-green-800 bg-transparent hover:bg-green-200 active:bg-green-300',
red: 'text-red-700 bg-transparent hover:bg-surface-red-3 active:bg-surface-red-4',
}[props.theme]

let focusClasses = {
const focusClasses = {
gray: 'focus-visible:ring focus-visible:ring-outline-gray-3',
blue: 'focus-visible:ring focus-visible:ring-blue-400',
green: 'focus-visible:ring focus-visible:ring-outline-green-2',
red: 'focus-visible:ring focus-visible:ring-outline-red-2',
}[props.theme]

let variantClasses = {
const variantClasses = {
subtle: subtleClasses,
solid: solidClasses,
outline: outlineClasses,
ghost: ghostClasses,
}[props.variant]

let themeVariant: ThemeVariant = `${props.theme}-${props.variant}`
const themeVariant: ThemeVariant = `${props.theme}-${props.variant}`

let disabledClassesMap: Record<ThemeVariant, string> = {
const disabledClassesMap: Record<ThemeVariant, string> = {
'gray-solid': 'bg-surface-gray-2 text-ink-gray-4',
'gray-subtle': 'bg-surface-gray-2 text-ink-gray-4',
'gray-outline':
Expand All @@ -157,7 +152,7 @@ const buttonClasses = computed(() => {
'bg-surface-red-2 text-ink-red-2 border border-outline-red-1',
'red-ghost': 'text-ink-red-2',
}
let disabledClasses = disabledClassesMap[themeVariant]
const disabledClasses = disabledClassesMap[themeVariant]

let sizeClasses = {
sm: 'h-7 text-base px-2 rounded',
Expand Down Expand Up @@ -186,51 +181,81 @@ const buttonClasses = computed(() => {
})

const slotClasses = computed(() => {
let classes = {
return {
sm: 'h-4',
md: 'h-4.5',
lg: 'h-5',
xl: 'h-6',
'2xl': 'h-6',
}[props.size]

return classes
})

const isDisabled = computed(() => {
return props.disabled || props.loading
})

const iconComponent = computed(() => getRenderableIcon(props.icon))
const iconLeftComponent = computed(() => getRenderableIcon(props.iconLeft))
const iconRightComponent = computed(() => getRenderableIcon(props.iconRight))

const isIconButton = computed(() => {
return props.icon || slots.icon || hasLucideIconInDefaultSlot.value
return Boolean(
iconComponent.value || slots.icon || hasLucideIconInDefaultSlot.value,
)
})

const hasLucideIconInDefaultSlot = computed(() => {
if (!slots.default) return false

const slotContent = slots.default()
if (!Array.isArray(slotContent)) return false
// if the slot contains only one element and it's a lucide icon
// render it as an icon button
let firstVNode = slotContent[0]
if (
typeof firstVNode.type?.name == 'string' &&
firstVNode.type?.name?.startsWith('lucide-')
) {
return true
}
return false

const firstVNode = slotContent[0]
return (
typeof firstVNode.type?.name === 'string' &&
firstVNode.type.name.startsWith('lucide-')
)
})
Comment on lines 207 to 218
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Potential TypeError when default slot returns an empty array.

slotContent[0] is undefined when a consumer passes an empty-returning default slot (e.g. default: () => [] or <template #default />). Accessing firstVNode.type?.name then throws Cannot read properties of undefined. The optional chain needs to start at firstVNode itself.

🛡️ Proposed fix 
   const firstVNode = slotContent[0]
   return (
-    typeof firstVNode.type?.name === 'string' &&
-    firstVNode.type.name.startsWith('lucide-')
+    typeof firstVNode?.type?.name === 'string' &&
+    firstVNode.type.name.startsWith('lucide-')
   )
📝 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
const hasLucideIconInDefaultSlot = computed(() => {
if (!slots.default) return false
const slotContent = slots.default()
if (!Array.isArray(slotContent)) return false
// if the slot contains only one element and it's a lucide icon
// render it as an icon button
let firstVNode = slotContent[0]
if (
typeof firstVNode.type?.name == 'string' &&
firstVNode.type?.name?.startsWith('lucide-')
) {
return true
}
return false
const firstVNode = slotContent[0]
return (
typeof firstVNode.type?.name === 'string' &&
firstVNode.type.name.startsWith('lucide-')
)
})
const hasLucideIconInDefaultSlot = computed(() => {
if (!slots.default) return false
const slotContent = slots.default()
if (!Array.isArray(slotContent)) return false
const firstVNode = slotContent[0]
return (
typeof firstVNode?.type?.name === 'string' &&
firstVNode.type.name.startsWith('lucide-')
)
})
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/components/Button/Button.vue` around lines 207 - 218,
hasLucideIconInDefaultSlot can throw when slots.default returns an empty array
because firstVNode is undefined; update the guard so you don't access properties
on undefined: check that slotContent[0] (firstVNode) exists before reading its
type/name (e.g., return false if !firstVNode), or use optional chaining starting
at firstVNode (firstVNode?.type?.name) and then test startsWith; reference
hasLucideIconInDefaultSlot, slots.default, slotContent, and firstVNode when
applying the change.


const handleClick = () => {
if (import.meta.env.DEV) {
watchEffect(() => {
warnForRuntimeStringIcon('icon', props.icon)
warnForRuntimeStringIcon('iconLeft', props.iconLeft)
warnForRuntimeStringIcon('iconRight', props.iconRight)
})
}

function getRenderableIcon(icon?: ButtonProps['icon']) {
if (!icon || typeof icon === 'string') return null
return icon
}

function warnForRuntimeStringIcon(
propName: 'icon' | 'iconLeft' | 'iconRight',
icon?: string | ButtonProps['icon'],
) {
if (!icon || typeof icon !== 'string') return

const key = `${propName}:${icon}`
if (warnedRuntimeStringIcons.has(key)) return

warnedRuntimeStringIcons.add(key)
console.warn(
`[frappe-ui] Button ${propName}="${icon}" requires the frappe-ui/vite transform or a component binding like :${propName}="LucideX".`,
)
}
Comment on lines +220 to +246
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find dynamic (v-bind / :icon) usages on Button that are not component references
rg -nP --type=vue -C2 '<Button\b[^>]*\s(?::|v-bind:)(?:icon|icon-left|icon-right|iconLeft|iconRight)\s*=' 

# Find call sites that forward a string-typed prop into Button icon props
rg -nP --type=vue -C2 '<Button\b[^>]*\s(?::|v-bind:)(?:icon|icon-left|icon-right|iconLeft|iconRight)\s*="[a-zA-Z_$][\w.$]*"'

Repository: frappe/frappe-ui

Length of output: 115

🏁 Script executed:

#!/bin/bash
# Find all .vue files and search for Button component with icon bindings
fd -e vue -x rg -l '<Button\b' {} \; | head -20

# Then search specifically for dynamic icon bindings in those files
echo "=== Dynamic icon bindings in Button usages ==="
fd -e vue -x grep -l '<Button' {} \; | xargs rg -C2 ':(icon|iconLeft|iconRight|icon-left|icon-right)\s*=' 2>/dev/null | head -50

# Also search for string-typed icon props being forwarded
echo "=== String icon prop forwarding patterns ==="
fd -e vue -x grep -E '<Button[^>]*(icon|iconLeft|iconRight)' {} \; 2>/dev/null | grep -E '"[a-zA-Z_]' | head -30

Repository: frappe/frappe-ui

Length of output: 4991

🏁 Script executed:

#!/bin/bash
# Find ButtonProps definition
rg 'interface ButtonProps|type ButtonProps' -A 15 src/components/Button/

Repository: frappe/frappe-ui

Length of output: 961

🏁 Script executed:

#!/bin/bash
# Get full ButtonProps icon property definition
rg 'icon\?' src/components/Button/types.ts -A 3

Repository: frappe/frappe-ui

Length of output: 128

🏁 Script executed:

#!/bin/bash
# Check CalendarWeekly and CalendarDaily for context
cat -n src/components/Calendar/CalendarWeekly.vue | sed -n '1,30p'
cat -n src/components/Calendar/CalendarDaily.vue | sed -n '1,30p'

Repository: frappe/frappe-ui

Length of output: 2625

🏁 Script executed:

#!/bin/bash
# Search for Button.vue to find what Phase 2/3 components are mentioned or planned
rg 'Phase|TODO|FIXME|migration' src/components/Button/ -i
rg 'DropdownOption|Input|Switch' src/components/ --type=vue -l | head -10

Repository: frappe/frappe-ui

Length of output: 87

🏁 Script executed:

#!/bin/bash
# Find all Button usages with string literal icons (not component references)
rg -U '<Button[^>]*\s(?:icon|iconLeft|iconRight|icon-left|icon-right)\s*=\s*["\x27][a-z\-]+["\x27]' --type vue -c

# Also find computed/dynamic string icons
rg -U 'iconRight\s*=\s*".*\?" ' src/components/ --type vue -l

Repository: frappe/frappe-ui

Length of output: 115

🏁 Script executed:

#!/bin/bash
# Search for any TODO, FIXME, or comments about the string icon deprecation
rg 'string.*icon|icon.*string|migration|Phase' src/components/Button/ -i -A 2 -B 2

Repository: frappe/frappe-ui

Length of output: 2754

🏁 Script executed:

#!/bin/bash
# List all components that use Button with potential string icons
fd -e vue -path "*/src/components/*" -x grep -l '<Button' {} \; | head -20

Repository: frappe/frappe-ui

Length of output: 228

Dynamic string icons in Button cause silent null rendering in production — migrate CalendarDaily and CalendarWeekly to component references.

getRenderableIcon returns null for any string, and the warning only fires in DEV builds. CalendarDaily.vue and CalendarWeekly.vue still pass dynamic strings (:iconRight="showCollapsable ? (isCollapsed ? 'chevron-down' : 'chevron-up') : ''"), which silently render nothing in production with no diagnostic.

The type signature still allows icon?: string | Component, enabling this unsafe pattern. Consider tightening ButtonProps.{icon,iconLeft,iconRight} to Component | undefined once all call sites are migrated, making the string path a compile error rather than a silent null.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/components/Button/Button.vue` around lines 220 - 246, getRenderableIcon
currently returns null for string icons and warnForRuntimeStringIcon only warns
in DEV, so dynamic string icon usage (e.g., in CalendarDaily/CalendarWeekly)
silently renders nothing in production; change the Button API and implementation
to disallow runtime strings: update the ButtonProps types (icon, iconLeft,
iconRight) to be Component | undefined (remove string), update getRenderableIcon
to accept only Component and return it directly, and remove or narrow DEV-only
warnForRuntimeStringIcon usage; migrate call sites (CalendarDaily,
CalendarWeekly) to pass actual component references instead of strings so the
type system prevents regressions.


function handleClick() {
if (props.route) {
return router.push(props.route)
} else if (props.link) {
}

if (props.link) {
return window.open(props.link, '_blank')
}
}

const rootRef = ref()
const rootRef = ref<HTMLElement | null>(null)
defineExpose({ rootRef })

defineSlots<{
Expand Down
18 changes: 15 additions & 3 deletions src/components/Button/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,25 @@ export interface ButtonProps {
/** Text label displayed inside the button */
label?: string

/** Icon shown when no left or right icon is specified */
/**
* Icon shown when no left or right icon is specified.
* Static literals like `icon="menu"` are rewritten to per-icon Lucide imports
* when using `frappe-ui/vite`.
*/
icon?: string | Component

/** Icon shown before the label */
/**
* Icon shown before the label.
* Static literals like `icon-left="menu"` are rewritten to per-icon Lucide
* imports when using `frappe-ui/vite`.
*/
iconLeft?: string | Component

/** Icon shown after the label */
/**
* Icon shown after the label.
* Static literals like `icon-right="menu"` are rewritten to per-icon Lucide
* imports when using `frappe-ui/vite`.
*/
iconRight?: string | Component

/** Tooltip text shown on hover */
Expand Down
54 changes: 33 additions & 21 deletions vite/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ export default defineConfig({
})
```

All plugins except `frappeTypes` are **enabled by default**. `frontendRoute`
and `frappeTypes` require explicit configuration — `frontendRoute` sets the app route,
and `frappeTypes` needs an `input` map of app names to doctype names. Pass
custom options to override any plugin, or `false` to disable it.
All plugins except `frappeTypes` are **enabled by default**. `frontendRoute` and
`frappeTypes` require explicit configuration — `frontendRoute` sets the app
route, and `frappeTypes` needs an `input` map of app names to doctype names.
Pass custom options to override any plugin, or `false` to disable it.

---

Expand All @@ -50,8 +50,8 @@ custom options to override any plugin, or `false` to disable it.
The route where your app is served (e.g. `'/g'`). This top-level option is
shared across plugins and controls:

- **Dev server site banner** — prints clickable URLs for all sites where the
app is installed on startup.
- **Dev server site banner** — prints clickable URLs for all sites where the app
is installed on startup.
- **Build output path** — `indexHtmlPath` is auto-inferred as
`../<appName>/www/<path>.html`.

Expand All @@ -73,10 +73,10 @@ instance.
- Proxies routes like `/api`, `/app`, `/assets`, `/files`, etc.
- Auto-detects the Frappe port from `common_site_config.json`

| Option | Description | Default |
| -------- | ---------------------------------------------- | ------------------------------------------------ |
| `port` | Vite dev server port | Auto-calculated from `webserver_port` |
| `source` | Regex for routes to proxy | `'^/(app\|login\|api\|assets\|files\|private)'` |
| Option | Description | Default |
| -------- | ------------------------- | ----------------------------------------------- |
| `port` | Vite dev server port | Auto-calculated from `webserver_port` |
| `source` | Regex for routes to proxy | `'^/(app\|login\|api\|assets\|files\|private)'` |

```javascript
frappeui({
Expand Down Expand Up @@ -106,15 +106,27 @@ standardized stroke-width of 1.5.
import LucideArrowRight from '~icons/lucide/arrow-right'
```

**Static `Button` icon props** — literal string props on `Button` are rewritten
at build time to direct Lucide imports, so the icon stays tree-shakeable:

```vue
<Button icon="menu" />
<Button icon-left="search" icon-right="chevron-down" />
```

This only applies to static string literals in `.vue` templates. Dynamic string
values are not transformed; prefer passing the imported Lucide component in
those cases.

### Frappe Types

Auto-generates TypeScript interfaces from Frappe DocType JSON files. Interfaces
are regenerated only when the source DocType changes.

| Option | Description | Default |
| -------- | ------------------------------------------ | ------------------------ |
| `input` | Map of `app_name` → array of doctype names | *(required)* |
| `output` | Output file path for generated types | `src/types/doctypes.ts` |
| Option | Description | Default |
| -------- | ------------------------------------------ | ----------------------- |
| `input` | Map of `app_name` → array of doctype names | _(required)_ |
| `output` | Output file path for generated types | `src/types/doctypes.ts` |

```javascript
frappeui({
Expand Down Expand Up @@ -166,13 +178,13 @@ directory structure.
- Sets correct base URLs for Frappe's asset serving
- Copies the built `index.html` to the specified location (typically in `www/`)

| Option | Description | Default |
| --------------- | ------------------------------------ | ----------------------------------------- |
| `outDir` | Build output directory | `'../app_name/public/frontend'` (auto) |
| `baseUrl` | Base URL for assets | `'/assets/app_name/frontend/'` (auto) |
| `indexHtmlPath` | Where to copy built `index.html` | Inferred from `frontendRoute` |
| `emptyOutDir` | Clear output directory before build | `true` |
| `sourcemap` | Generate source maps | `true` |
| Option | Description | Default |
| --------------- | ----------------------------------- | -------------------------------------- |
| `outDir` | Build output directory | `'../app_name/public/frontend'` (auto) |
| `baseUrl` | Base URL for assets | `'/assets/app_name/frontend/'` (auto) |
| `indexHtmlPath` | Where to copy built `index.html` | Inferred from `frontendRoute` |
| `emptyOutDir` | Clear output directory before build | `true` |
| `sourcemap` | Generate source maps | `true` |

```javascript
frappeui({
Expand Down
Loading
Loading