diff --git a/.changeset/lucky-pugs-shake.md b/.changeset/lucky-pugs-shake.md new file mode 100644 index 0000000000..71af6fb8c7 --- /dev/null +++ b/.changeset/lucky-pugs-shake.md @@ -0,0 +1,9 @@ +--- +"@object-ui/components": patch +--- + +`ui:dropdown-menu` now resolves a menu item's authored `icon` to a glyph instead of drawing the name as raw text. + +Both arms of the item renderer — `DropdownMenuItem` and `DropdownMenuSubTrigger` — rendered `icon` straight into a text node, so an item authored as `{ "label": "Copy", "icon": "copy" }` drew the literal word `copy` beside its label. The name is now resolved through `resolveIcon`, the same lucide **record** surface `ui:button` and the `action:*` family already resolve against: a live name draws its glyph, and an unknown or retired spelling draws nothing rather than degrading to a wrong glyph. + +The `components-overlay-dropdown-menu/with-icons` catalog fixture declared the retired lucide spelling `edit`, which is absent from lucide's runtime `icons` record and would therefore have drawn no glyph; it now declares `square-pen`, the live key the retired export resolves to by identity. diff --git a/content/docs/components/overlay/dropdown-menu.mdx b/content/docs/components/overlay/dropdown-menu.mdx index 9ae5380d06..040e115b3d 100644 --- a/content/docs/components/overlay/dropdown-menu.mdx +++ b/content/docs/components/overlay/dropdown-menu.mdx @@ -13,13 +13,21 @@ The Dropdown Menu component displays a list of actions or options when triggered +## Icons + +An item's `icon` is a **kebab-case Lucide icon name**, resolved against lucide's +runtime `icons` record — the same surface `ui:button` and the `action:*` family +resolve against. A name that is not a live key of that record (an unknown or a +retired spelling such as `edit`) renders **no glyph**, never a fallback glyph +and never the literal name as text. + ## Schema ```plaintext interface DropdownMenuItem { label?: string; value?: string; - icon?: string; + icon?: string; // kebab-case Lucide icon name (e.g. "square-pen") variant?: 'default' | 'destructive'; type?: 'separator'; disabled?: boolean; diff --git a/examples/schema-catalog/src/schemas/components-overlay-dropdown-menu/with-icons.json b/examples/schema-catalog/src/schemas/components-overlay-dropdown-menu/with-icons.json index cac1ac1fd3..a39cab428b 100644 --- a/examples/schema-catalog/src/schemas/components-overlay-dropdown-menu/with-icons.json +++ b/examples/schema-catalog/src/schemas/components-overlay-dropdown-menu/with-icons.json @@ -9,7 +9,7 @@ { "label": "Edit", "value": "edit", - "icon": "edit" + "icon": "square-pen" }, { "label": "Copy", diff --git a/packages/components/src/__tests__/dropdown-menu-item-icon.test.tsx b/packages/components/src/__tests__/dropdown-menu-item-icon.test.tsx new file mode 100644 index 0000000000..959d480d24 --- /dev/null +++ b/packages/components/src/__tests__/dropdown-menu-item-icon.test.tsx @@ -0,0 +1,134 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * `ui:dropdown-menu` resolves an item's authored `icon` to a glyph (objectui#5930). + * + * Before the fix both arms of `renderMenuItems` rendered the authored STRING + * into a text node — `{item.icon && {item.icon}}` — + * so the catalog fixture literally named `with-icons.json` drew the words + * `edit Edit`, `copy Copy`, `trash Delete`. The key parsed, published and was + * documented in the registration's own item-shape description; nothing was + * red, because no renderer test asserted a glyph here. + * + * ## Why the assertions are shaped this way + * + * The defect is a WRONG RENDER, not an absent one, so `queryByText(name)` is + * the load-bearing assertion: a test that only asserted "an svg exists" passes + * against the broken renderer too, since the pre-fix `` sits inside the + * same item as the trigger's own glyph. Both directions are asserted — the + * glyph appears AND the bare name does not. + * + * ⚠️ `defaultOpen: true` is load-bearing. Radix mounts `DropdownMenuContent` + * lazily, so without it this suite renders an EMPTY container and proves + * nothing — the same trap recorded on `inline-locale-label-read-sites.test.tsx`. + * + * ## Why the renderer is invoked DIRECTLY + * + * `ComponentRegistry.get(name)` returns the component the registry actually + * renders; driving through `SchemaRenderer` injects its own props around it and + * can be green in both directions (PR #4603's toggle case, restated by #4580). + * + * ## Why lucide is NOT mocked here + * + * The contract under test is "the authored name is resolved against lucide's + * runtime `icons` RECORD", and the record surface is a synchronous object + * lookup with no chunk-loading path to flake on — unlike the dynamic + * (`LazyIcon`) surface, which sibling suites mock for exactly that reason. + * Mocking the record here would delete the half of the contract that matters: + * that a RETIRED spelling resolves to nothing. `edit` is that control — it is + * a deprecated lucide export whose key was dropped from the record, so it is + * the case that must render no glyph while `square-pen` (the same object under + * its live key) must render one. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import { ComponentRegistry } from '@object-ui/core'; +// Registers the renderers at module scope, NOT inside a `beforeAll` — there the +// cold transform is billed to `hookTimeout`. See +// object-ui/no-dynamic-import-in-test-hook (objectui#3010/#3021). +import '../renderers'; + +afterEach(() => cleanup()); + +/** `defaultOpen` is load-bearing — see the header note on Radix's lazy mount. */ +function renderMenu(items: any[]) { + const C = ComponentRegistry.get('dropdown-menu') as React.ComponentType; + return render( + , + ); +} + +/** The rendered glyph for an item, located via the item's own label. */ +function glyphFor(label: string): SVGElement | null { + const item = screen.getByText(label).closest('[role="menuitem"], [role="menu"] div'); + return item?.querySelector('svg') ?? null; +} + +describe('ui:dropdown-menu item icon resolution (objectui#5930)', () => { + describe('DropdownMenuItem arm', () => { + it('renders a glyph for a live icon name', () => { + renderMenu([{ label: 'Copy', icon: 'copy' }]); + expect(glyphFor('Copy')).not.toBeNull(); + }); + + it('does NOT render the authored icon name as text — the defect itself', () => { + renderMenu([{ label: 'Copy', icon: 'copy' }]); + // Pre-fix this found the literal word beside the label. + expect(screen.queryByText('copy')).toBeNull(); + }); + + it('renders no glyph and no text for a RETIRED spelling', () => { + // `edit` is a deprecated lucide export dropped from the runtime `icons` + // record. The record surface is chosen precisely so this renders nothing + // rather than degrading to a wrong glyph. + renderMenu([{ label: 'Edit', icon: 'edit' }]); + expect(glyphFor('Edit')).toBeNull(); + expect(screen.queryByText('edit')).toBeNull(); + }); + + it('renders no glyph when no icon is authored', () => { + renderMenu([{ label: 'Plain' }]); + expect(glyphFor('Plain')).toBeNull(); + }); + }); + + describe('DropdownMenuSubTrigger arm', () => { + // The submenu TRIGGER is mounted with the parent content; only + // `DropdownMenuSubContent` needs the submenu opened, and it is not read here. + it('renders a glyph for a live icon name', () => { + renderMenu([{ label: 'More', icon: 'trash', children: [{ label: 'Nested' }] }]); + expect(glyphFor('More')).not.toBeNull(); + }); + + it('does NOT render the authored icon name as text — the defect itself', () => { + renderMenu([{ label: 'More', icon: 'trash', children: [{ label: 'Nested' }] }]); + expect(screen.queryByText('trash')).toBeNull(); + }); + }); + + describe('the with-icons.json catalog fixture', () => { + // The fixture is a live specimen AND a declared AI few-shot retrieval + // source, so every name it ships must actually draw. `square-pen` is the + // identity-derived live key for the retired `edit` this fixture carried. + it('draws a glyph for every icon name it declares', () => { + renderMenu([ + { label: 'Edit', value: 'edit', icon: 'square-pen' }, + { label: 'Copy', value: 'copy', icon: 'copy' }, + { label: 'Delete', value: 'delete', icon: 'trash' }, + ]); + for (const label of ['Edit', 'Copy', 'Delete']) { + expect(glyphFor(label), `${label} should draw a glyph`).not.toBeNull(); + } + for (const name of ['square-pen', 'copy', 'trash']) { + expect(screen.queryByText(name)).toBeNull(); + } + }); + }); +}); diff --git a/packages/components/src/renderers/overlay/dropdown-menu.tsx b/packages/components/src/renderers/overlay/dropdown-menu.tsx index 69558ad8ed..f8f3b942d2 100644 --- a/packages/components/src/renderers/overlay/dropdown-menu.tsx +++ b/packages/components/src/renderers/overlay/dropdown-menu.tsx @@ -26,6 +26,18 @@ import { DropdownMenuSubContent } from '../../ui'; import { renderChildren } from '../../lib/utils'; +// Same-package sibling import, the path `renderers/complex/data-table.tsx` +// already uses. `icon` on a menu item is an authored lucide NAME, and it was +// rendered as a raw text node here — the fixture named `with-icons.json` drew +// the words "edit"/"copy"/"trash" beside its labels (objectui#5930). +// +// Routed through the RECORD surface (`icons` from 'lucide-react'), which is +// what `action:*` and `ui:button` next door resolve against, so a retired +// spelling renders NOTHING rather than a word. The dynamic surface +// (`LazyIcon`) is deliberately NOT used: it degrades an unknown name to the +// `Database` glyph, trading a no-icon failure for a WRONG-icon one, recorded +// as ruled out for authored icon fields by objectui#5622 and #5633. +import { resolveIcon } from '../action/resolve-icon'; // Helper for recursive menu items const renderMenuItems = (items: any[]) => { @@ -33,11 +45,15 @@ const renderMenuItems = (items: any[]) => { return items.map((item: any, i: number) => { if (item.type === 'separator') return ; if (item.type === 'label') return {item.label}; + // Resolved once per item and read by BOTH arms below. The submenu-trigger + // arm carried the identical defect; repairing only the leaf would be a + // narrower version of the same bug (objectui#5930). + const Icon = resolveIcon(item.icon); if (item.children) { return ( - {item.icon && {item.icon}} + {Icon && } {item.label} @@ -49,7 +65,7 @@ const renderMenuItems = (items: any[]) => { return ( - {item.icon && {item.icon}} + {Icon && } {item.label} {item.shortcut && {item.shortcut}} @@ -96,7 +112,7 @@ ComponentRegistry.register('dropdown-menu', name: 'items', type: 'array', label: 'Items', - description: 'Recursive structure: { type?: "separator"|"label", label, icon, shortcut, disabled, children: [] }' + description: 'Recursive structure: { type?: "separator"|"label", label, icon, shortcut, disabled, children: [] }. `icon` is a kebab-case Lucide icon name resolved against lucide\'s runtime `icons` record; an unknown or retired spelling renders no glyph.' }, { name: 'className', type: 'string', label: 'Content CSS Class' } ],