Dropdown: standardize node slots on inlineStartNode/inlineEndNode#98
Merged
Conversation
Rename DropdownItem icon -> leadingIcon and endIcon -> trailingIcon, and drop the value text slot (callers compose value text into trailing) plus its with-value variant. Rename DropdownSubTrigger icon -> leadingIcon and DropdownLabel action -> trailing so every slot follows the same leading*/trailing* vocabulary used by Button and Input. Update the dropdown, toolbar, and breadcrumb consumers on main to match.
|
📚 Storybook preview: https://pr-98-propel-storybook.vamsi-906.workers.dev |
There was a problem hiding this comment.
Pull request overview
Standardizes Dropdown slot prop names to match the library’s leading/trailing conventions (e.g. Button/Input), and simplifies DropdownItem’s trailing-side API by removing the dedicated value slot in favor of composing into trailing. This is a breaking public API change affecting component consumers and stories.
Changes:
- Rename
DropdownItemprops:icon→leadingIcon,endIcon→trailingIcon, removevalue, removewith-valuelayout variant. - Rename
DropdownSubTriggerprop:icon→leadingIcon; renameDropdownLabelprop:action→trailing. - Update Storybook consumers (Dropdown + Toolbar stories) to use the new prop names and compose prior
valuecontent intotrailing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/propel/src/components/toolbar/toolbar.stories.tsx | Updates toolbar dropdown story usage from icon to leadingIcon. |
| packages/propel/src/components/dropdown/index.tsx | Renames public slot props, removes with-value / value, and updates rendering to the new slot names. |
| packages/propel/src/components/dropdown/dropdown.stories.tsx | Updates dropdown stories to the renamed props and recomposes old value content into trailing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…on/trailing Aligns the Dropdown slots with the library's node-slot convention (Button/IconButton use inlineStartNode/inlineEndNode + nodeSlotClass + --node-size), instead of the leadingIcon/leading/trailing/trailingIcon/icon/value/action names. Each menu part now exposes a single logical inlineStartNode + inlineEndNode: - DropdownItem: leadingIcon + leading -> inlineStartNode; trailing + trailingIcon -> inlineEndNode (value was already folded into trailing). - DropdownCheckboxItem: icon -> inlineStartNode; value -> inlineEndNode. - DropdownSubTrigger: leadingIcon -> inlineStartNode; trailing -> inlineEndNode. - DropdownLabel: trailing -> inlineEndNode. Icons in a slot size to --node-size (16px) and inherit the row color; full-size controls (Checkbox/Avatar/swatch) size themselves. Stories updated to match.
lifeiscontent
changed the title
bhaveshraja
approved these changes
Jun 15, 2026
lifeiscontent
added a commit
that referenced
this pull request
Jun 16, 2026
lifeiscontent
added a commit
that referenced
this pull request
Jun 16, 2026
lifeiscontent
added a commit
that referenced
this pull request
Jun 16, 2026
lifeiscontent
added a commit
that referenced
this pull request
Jun 16, 2026
* Add Table component * Table: use div wrappers in th/td; gate sort button on onSort TableHead/TableCell wrapped children in a <span>, which restricts them to phrasing content and produces invalid HTML when block-level children are passed into <th>/<td>. Switch to <div> wrappers. Also, a sortable header without an onSort handler rendered a focusable <button> that did nothing. Only render the sort control when onSort is provided; otherwise fall back to a plain header label. * Table: remove cva defaultVariants; make TableHead variant required The TableHead variant axis (default/sortable) no longer defaults to "default" via cva. variant is now a required prop, so every header cell declares its visual treatment explicitly. sort/onSort remain optional (additive, only meaningful for sortable headers). * Table stories: add Figma design link to meta parameters * Table: add Spreadsheet variant + editable cells via Dropdown Address design review on PR #15 (Figma 4017-653): - Two variants on the root `Table` via a required `variant` prop, read by the cells through context: `table` (row dividers only) and `spreadsheet` (full grid — every cell bordered). Both share the Figma cell metrics (38px, px-3 py-2, header on layer-1, body on surface-1) and a rounded outer border. - `TableEditableCell`: a click-to-edit cell whose value + trailing chevron is the trigger for a propel Dropdown, picking a new value that updates the row in place (Figma "Account type" editable cell). Owns the Dropdown root + trigger; the consumer passes the DropdownContent. - Stories: Default (Table), Spreadsheet, Sortable, EditableCells (click-cell → dropdown → value updates), all with the 4017-653 design link. Pagination is intentionally left as a separate component to compose, per review. * Table: add keyboard play tests for sortable header and editable cell SortableKeyboard: Tab to the sortable header button, then Enter/Space cycle the sort (none -> ascending -> descending -> none) with aria-sort and onSort following along; asserts the th scope=col semantics. EditableCellKeyboard: Tab+Enter opens the portaled Dropdown, Arrow Down + Enter selects a value and the cell updates in place, and Escape closes the menu and returns focus to the cell trigger. The portaled menu is queried from the document body by unique item text with waitFor for open/close. Both tagged [!dev,!autodocs,!manifest] so they run under test only. * Table: add a WithPagination story composing the Pagination component Designer follow-up on #15: pagination can be used with the table. The Table and Pagination stay separate components, so the story keeps the full dataset, renders only the current page of rows, and lets the Pagination below drive page and page size (changing the size resets to page 1 and updates the range label). Verified in Storybook (table variants, editable cell, and pagination) plus a play test that advances a page and asserts the rows swap. * Storybook: drop the ai-generated tag from the table story * Table: fence the TableEditableCell TSDoc usage example as a code block Same Markdown-in-autodocs issue as Popover/Tabs/Dropdown: the indented JSX example let the wrapper tag leak out of the code box. Wrap it in a ```tsx fence. * Table: 44px rows, fix editable-cell height, stable pagination width, cell states Addresses design review on #15: - Rows are 44px. The editable cell was forcing rows to ~55px because its p-0 override lost to the shared cell padding under cx; padding now lives on each cell type, so the editable cell is truly full-bleed and its 44px button sets the height. - Editable cells get an explicit open/selected state (data-[popup-open]) next to the existing hover and focus states. - The WithPagination demo gets a fixed width so the table no longer resizes between pages (it is w-full and was shrink-to-fitting each page's content under centered layout). The table already has the border-subtle outer border. * Table: outer border, sticky header + pinned columns, cell slots, action cell (#15) Addresses the design review (Figma 5196-4084): 1. The outer border-subtle now shows: the table is wrapped in a rounded, bordered, scrollable frame (a <table> can't clip its own rounded corners, so the border was invisible before). 2. Sticky header on vertical scroll, plus an opt-in pinned first/last column on horizontal scroll via each cell's pinned start/end. The frame is keyboard-scrollable (tabindex) only while it overflows, so it adds no stray tab stop and satisfies the axe scrollable-region rule. 3. TableCell gains inlineStartNode/inlineEndNode slots for a leading/trailing icon or Avatar (the Name column). 4. New TableActionCell: an icon-only (default ellipsis) row-options cell that opens a Dropdown, mirroring TableEditableCell. 5. Rows sit on layer-2 / layer-2-hover; actionable cells are layer-transparent / layer-transparent-hover so the cell hover reads distinctly over the row hover. Closes #15 * Table: wait for the action menu to settle before asserting visibility The RichRows action-menu test asserted toBeVisible right after findByRole, racing the menu's open scale-fade in CI. Wrap it in waitFor like the other menu tests. * Table: update RichRows DropdownItem to inlineStartNode (follows #98) * Table: address design review (single grid border, tertiary header, overlay scrollbar, selected cell) Round of fixes from the PR #15 design review: - Spreadsheet variant no longer draws a double border. The scroll frame owns the single outer ring, so cells only draw interior dividers (inline-end + bottom, with the last column/row dropping theirs). This also clears the squared-off corners that poked past the rounded frame. - Header text now uses text-tertiary to match the Figma "Table header" token (was text-secondary on plain headers). - Scroll frame is now a Base UI ScrollArea, so the scrollbar overlays the content instead of reserving a 12px gutter. The header is no longer clipped and the table width stays constant whether or not the scrollbar is showing (also keeps width stable across pagination). Sticky header + pinned columns still work because the ScrollArea Viewport is the real scroll container; it also gets tabIndex=0 only while it overflows, which keeps the scrollable-region-focusable a11y rule satisfied without the manual ResizeObserver. - Pinned columns draw a hairline on their inline edge so they read as separated from the content scrolling under them (logical border, RTL-safe). - Editable cell gains a selected prop: a persistent layer-transparent-selected tint a step above hover, for marking the active cell. The story wires it to the last-opened cell. Verified in the browser across light/dark and LTR/RTL: single clean grid, tertiary header, centered avatars, 44px rows, overlay scrollbar (0 reserved space), constant width across pages, sticky header + pinned columns. vp check and the full test suite (736 tests, axe gate across 4 themes) pass. * Table: dim editable/action cell icons when the trigger is disabled * Table: use px-4 cell padding to match Figma spec
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #61.
DropdownItemwas the worst offender for slot-naming soup. This aligns every Dropdown menu part with the library's node-slot convention (Button/IconButton useinlineStartNode/inlineEndNode+nodeSlotClass+--node-size), notleadingIcon/leading/trailing/trailingIcon/icon/value/action. Each part now exposes a single logical start + end node.API
icon+leadingbecomeinlineStartNode;value+trailing+endIconbecomeinlineEndNode. Keepslabel/description/secondaryText/selected/variant/emphasis.iconbecomesinlineStartNode;valuebecomesinlineEndNode(the Checkbox stays the fixed leading control).iconbecomesinlineStartNode;trailingbecomesinlineEndNode.actionbecomesinlineEndNode.Icons dropped into a slot size to
--node-size(16px) and inherit the row color; full-size controls (Checkbox/Avatar/swatch) size themselves. All consumers onmain(dropdown / toolbar / breadcrumb stories) updated.Notes
DropdownItemwithicon=in its RichRows story; it needs a one-line follow-up toinlineStartNode=when it merges.Verification
vp check+vp run testgreen (167 tests incl. dropdown/toolbar/breadcrumb + axe gate).Closes #61