Skip to content

feat(selector): configurable indicator position for Selector and MultiSelector - #4993

Merged
cixzhang merged 4 commits into
mainfrom
feat/selector-indicator-position
Aug 14, 2026
Merged

feat(selector): configurable indicator position for Selector and MultiSelector#4993
cixzhang merged 4 commits into
mainfrom
feat/selector-indicator-position

Conversation

@cixzhang

@cixzhang cixzhang commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Why

Where the selection indicator sits in a dropdown row is a product decision, not a system one — a menu-style surface wants the mark leading the label, a form-style list wants it trailing — and today it is hard-coded, differently, in each selector: Selector draws its check at the end of the row, MultiSelector draws its checkbox at the start. Neither can be changed without renderOption gymnastics.

This adds indicatorPosition="start" | "end" to both. It stays a prop rather than a theme key: everything defineTheme addresses today is a style value or a component swap, and a layout switch would be the first prop-shaped entry in a theme — worth deciding deliberately, separately, and not as a side effect of this change.

What

Both selectors take indicatorPosition, logical (start follows RTL, verified below), and both default to what they render todayend for Selector's check, start for MultiSelector's checkbox. No existing call site changes appearance, and the end path in Selector emits the same markup it always has.

The one subtlety is Selector's mark column. CheckIndicator deliberately draws nothing when unchecked, so a row's geometry otherwise depends on whether it is the chosen one: on main today the chosen row's content box measures 200px against its neighbours' 224px, so it truncates 24px earlier. Every row now reserves the column, at either position, sized min-width rather than a fixed width so a theme that swaps check for a larger indicator (RadioIndicator is 20px at sm) still lines up.

ComplexSelector is untouched: it has a trigger and arbitrary popover content, no option rows and no selection mark.

Risk

Low, and additive. The defaults preserve today's rendering in both components; the only new DOM is the reserved column, and only when a caller asks for start.

Two naming notes for the record. First, indicatorPosition is about the selection mark, not the trigger chevron — which our theme targets confusingly call selector-indicator-icon. The Indicator module is the codebase's canonical meaning of "indicator", so the prop follows it, but the collision is real and the icon targets are the ones I'd rename eventually. Second, Design Conventions says combobox-like components mark selection with a checkmark at the end; that stays the default and the documented house style — this prop is the deviation, not a new default. If we want the same knob on Typeahead and CommandPalette (both hand-roll a trailing check icon rather than going through the indicator layer), that's a natural follow-up.

Testing

Seven unit tests across the two components: default and overridden order in each, the reserved column present on every row at start and on no row at end, a themed RadioIndicator positioned the same way, and the select-all row following the option rows.

Verified in Chromium against Storybook, not inferred from styles:

  • Selector at start — all three labels start at the same x (232px), mark at 208px on the chosen row only.
  • Selector at end — pixel-identical to main, same popup width (373px), with the per-row content box now uniform at 200px against main's 224/200/224.
  • check swapped for RadioIndicator at start — the column grows to the radio's 20px and labels stay aligned.
  • MultiSelector at end — every checkbox flush to the row's trailing edge (434px on a row ending at 466px), select-all included.
  • RTL (direction: rtl global) — start renders on the right, end on the left, in both components.

New stories: Selector › StartIndicatorPosition, MultiSelector › EndIndicatorPosition.

One thing this turned up

The MultiSelector story is the first one in the repo to open a MultiSelector popup by default, and that immediately failed pr-a11y with a critical violation — a role="separator" Divider sitting inside role="listbox", which only permits option and group. It is pre-existing (the select-all divider, plus every section/divider option in both selectors), and invisible until now because the audit never opens a closed popup. Filed as #4994 rather than fixed here — it needs a decision about section headings that has nothing to do with this prop. The story simply omits hasSelectAll so this PR is not gated on it; the select-all ordering is still covered by a unit test.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 13, 2026
@vercel

vercel Bot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Ready Ready Preview Aug 13, 2026 11:40pm

Request Review

@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

Modified Components

Indicator (@astryxdesign/core) · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 557 -
Complexity N/A High (26) -
MultiSelector (@astryxdesign/core) · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 1442 -
Complexity N/A Very High (182) -
Selector (@astryxdesign/core) · View in Storybook
Metric Before After Delta
Bundle Size (ESM) N/A N/A N/A
Lines of Code N/A 1491 -
Complexity N/A Very High (155) -

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/core N/A 4.7KB 1.2KB

Accessibility Audit

Status: No accessibility violations detected.


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

github-actions Bot added a commit that referenced this pull request Aug 13, 2026
github-actions Bot added a commit that referenced this pull request Aug 13, 2026
Comment thread packages/core/src/Selector/Selector.tsx Outdated
const content =
indicatorPosition === 'start' ? (
<>
<span {...stylex.props(styles.itemMarkColumn)}>{mark}</span>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why only on start side?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, and it was worse than asymmetric — it was papering over an inconsistency that already exists on main.

The column is now rendered at both positions, so every row has the same two children regardless of position or selection. Measured on the default (end) story, content box per row:

Apple Banana (chosen) Cherry
main 224px 200px 224px
this PR 200px 200px 200px

So today the chosen row is already 24px narrower than its neighbours — it truncates earlier — because the check only exists on that one row. Reserving on both sides fixes that, and it's why end was never really "unchanged markup" worth preserving.

Rendering is otherwise inert: the default story is pixel-identical to main before and after, and the popup measures the same width (373px) whether the longest label is the chosen row or not.

Two things I decided against, for the record. A CSS grid track on the row would reserve the column with no extra DOM node, but it changes the layout model on the path every existing caller is on, for a wrapper that costs one span per row. And the span deliberately carries no theme target — selector-check stays on the glyph, so the one-element-per-mark property from #4846 is intact. The tradeoff: a theme that hides the mark (selector-check: {display: none}) now leaves 24px reserved on every row instead of on the chosen one — consistent, but not zero. Say the word if you'd rather that case collapse and I'll give the column its own target.

MultiSelector already had this property — its checkbox wrapper renders in both positions, and the box draws in both states — so only the end push differs there.

@cixzhang cixzhang left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's make sure rendering for the item is consistent.

github-actions Bot added a commit that referenced this pull request Aug 13, 2026
@cixzhang
cixzhang merged commit e144ea7 into main Aug 14, 2026
22 of 23 checks passed
@github-actions
github-actions Bot deleted the feat/selector-indicator-position branch August 14, 2026 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. needs:design-review Affects visuals — Design should review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant