Feat/search dropdown#114
Conversation
tmccoy14
left a comment
There was a problem hiding this comment.
Review of the searchable-dropdown feature. Lint, typecheck, and prettier pass; the inline comments below are compliance/cleanup items grouped roughly must-fix → nits.
Two conceptual items that don't map cleanly to a single line:
role="menu"composition: the search<input>renders as a descendant ofDropdownMenuPrimitive.Content(role="menu"), which per ARIA should contain onlymenuitem-role descendants. Worth a deliberate decision on the SR pattern (a combobox/listbox composition is the usual guidance).- Submenu flattening is brittle by design: matching on
displayName === 'DropdownMenuSubContent'and only finding a directly-descendedSubContentfails silently if a consumer wraps/aliases it. Already noted in the docs, so acceptable — just flagging.
|
|
||
| /* | ||
| * TODO: Add searchPlaceholder and emptyPlaceholder to docs | ||
| */ |
There was a problem hiding this comment.
Leftover TODO + the docs were never updated. searchPlaceholder/emptyPlaceholder are new public props but filter-dropdown.mdx's props table doesn't list them. CLAUDE.md requires each component's MDX to stay in sync. Add both props to the table, then delete this comment.
|
|
||
| /* | ||
| * TODO: Add searchPlaceholder and emptyPlaceholder to the docs | ||
| */ |
There was a problem hiding this comment.
Same as filter-dropdown: leftover TODO and radio-dropdown.mdx is missing the new searchPlaceholder/emptyPlaceholder props. Update the docs table and remove the TODO.
| import { useState } from "react"; | ||
| import { | ||
| Avatar, | ||
| AvatarFallback, |
There was a problem hiding this comment.
Avatar and AvatarFallback (lines 3-4) are imported but never used — the demo uses Icon. ESLint flags both as warnings. Remove them.
| return ( | ||
| <div className={styles['dropdown-menu-search']}> | ||
| {icon ?? <SearchIcon className={styles['icon-size']} />} | ||
| <input |
There was a problem hiding this comment.
Accessibility: the search <input> has no accessible name — it relies solely on placeholder, which isn't a reliable accessible name. Add an aria-label (default it from placeholder, allow override). CLAUDE.md calls out accessible components specifically.
| const query = ctx?.query.trim() ?? ''; | ||
| if (!ctx || !query || ctx.matchCount > 0) return null; | ||
| return ( | ||
| <div className={cn(styles['dropdown-menu-empty'], className)} {...props}> |
There was a problem hiding this comment.
Accessibility: DropdownMenuEmpty is a plain div, so screen-reader users aren't told when filtering empties the list. Consider role="status" / aria-live="polite" so the empty state is announced.
| */ | ||
| const DropdownMenuSearch = ({ | ||
| className, | ||
| placeholder = 'Search…', |
There was a problem hiding this comment.
Ellipsis inconsistency: this default uses Search… (U+2026) but consumers use Search filters... / Search options... (three periods), and the demo mixes both (Search members... vs Search tools…). search-bar uses Search.... Pick one convention repo-wide.
| </DropdownMenuSubContent> | ||
| </DropdownMenuSub> | ||
| <DropdownMenuContent align="start"> | ||
| <DropdownMenuSearch placeholder="Search tools…" /> |
There was a problem hiding this comment.
Docs ordering: the "With Submenu" example (and its live demo) now includes DropdownMenuSearch/DropdownMenuEmpty, but search isn't introduced until the later "Search & filtering" section and this section's prose is only about nesting. Either keep this example search-free or add a forward reference.
| reveal: (seed: string) => void; | ||
| query: string; | ||
| setQuery: (value: string) => void; | ||
| /* Re focus the search input from outside DropdownMenuSearch */ |
There was a problem hiding this comment.
Nit: typo "Re focus" → "Re-focus".
| if (!ctx || !ctx.enabled) return; | ||
| ctx.registerItem(id, visible); | ||
| return () => ctx.unregisterItem(id); | ||
| }, [ctx, ctx?.enabled, id, visible]); |
There was a problem hiding this comment.
Efficiency: this effect lists ctx as a dependency, but the context value memo changes identity on every keystroke (its deps include query and matchCount). So every visible item unregisters + re-registers + triggers a full recount() on each keystroke — O(n²) per keystroke. Depend on the stable registerItem/unregisterItem/enabled values instead of the whole ctx. Fine for small menus, just noting.
| /* | ||
| * Search input - renders the search input and manages the search context | ||
| */ | ||
| const DropdownMenuSearch = ({ |
There was a problem hiding this comment.
Minor API inconsistency: DropdownMenuSearch doesn't forwardRef while every sibling part does, so consumers can't get a ref to the input. Consider forwarding it for parity.
Added search to DropdownMenu
Adds an optional search box to our existing
DropdownMenuso you can filter items in place. It's opt-in per menu — if a menu doesn't have a<DropdownMenuSearch />in it, nothing changes and Radix's normal typeahead still works. Everything is wired through a small internal context, so the usage stays the same, you just drop new parts intoDropdownMenuContent.What's new
<DropdownMenuSearch />— a search input you put insideDropdownMenuContent. Hidden by default and reveals when you start typing. PassalwaysVisibleif you want it showing straight away.<DropdownMenuEmpty />— a "no results" row that only shows when the query doesn't match any items.DropdownMenuItem/CheckboxItem/RadioItemhide themselves when they don't match. Matches ontextValue, falling back to the item's text.Usage
If an item has an icon/avatar instead of plain text, give it a
textValueso it filters properly.Heads up
Most of this is working around how Radix handles focus and keyboard:
Contentand callingpreventDefault()— Radix bails when the event's already defaultPrevented.SubContent's children inline while searching — Radix keeps sub items in a separate popover that isn't mounted during a top-level search, so there's no other way to filter them.Known gaps
DropdownMenuSubContent— wrapping it breaks it.Extras
DropdownMenuLabelstyling, since it looked the same as a clickable item making it confusing.<DropdownMenuSearch />to bothRadioDropdownandFilterDropdowncomponents. We need to update the docs for them afterfeat/docsget's merged.SortSelectorcomponent too @Shrinks99 ??Screen.Recording.2026-07-14.at.5.33.05.PM.mov
Linear ticket