feat(multi-selector): themeable dropdown option target with states and size - #4628
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
community
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsMultiSelector (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
| styles.itemLabel, | ||
| item.disabled && styles.itemLabelDisabled, | ||
| {...mergeProps( | ||
| themeProps('multi-selector-label'), |
There was a problem hiding this comment.
We may want this a bit more specific like 'multi-selector-option-label' so it doesn't get confused with the other label.
| <div | ||
| inert | ||
| {...mergeProps( | ||
| themeProps('multi-selector-checkbox'), |
There was a problem hiding this comment.
Similarly 'multi-selector-option-checkbox'
9a23c16 to
a6579a2
Compare
a6579a2 to
9122190
Compare
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
🔴 Blocking — four of the five new theme targets sit on layout-only elements
or expose internal structure (MultiSelector.tsx:1286-1291, :1304-1311,
:1349-1353, :1272-1277); Theming Infrastructure Principles 1, 4 and 5.
Thanks for the rename to -option-checkbox / -option-label — that took the
earlier feedback. Two things follow up on it: the description table still lists
the old -checkbox / -label names, and my open question from this morning
("mostly layout class properties … what's the goal for the theming?") is really
the crux of the whole PR, so let me make it concrete.
The frame I'm applying (and want to write into the wiki): a theming target
is a paint seam — color, background, border, font, radius, shadow. It is not a
layout seam — display, position, flex, padding, gap, transform, size. Layout
is the component's structural contract: it's what guarantees overflow behavior,
hit-target size, focus order and RTL, and it's exactly what a DOM refactor is
entitled to change. Exposing a layout seam freezes the structure and makes those
guarantees theme-dependent. Layout is themeable today, but only through the
declared derived-var / container pipeline (--_dropdown-menu-padding,
--_card-radius), where the component still owns the structure — never via a
raw class on an internal element. This isn't written down yet, so I'm raising it
as direction rather than citing a rule.
Target by target:
| Target | The element's own style | Call |
|---|---|---|
-option |
styles.item (:302): borderRadius, backgroundColor, border |
✅ keep — paint, and it converges with selector-option |
-option-checkbox |
checkboxDecorative (:323): pointerEvents/display/flexShrink |
🔴 layout, on an inert wrapper |
-option-label |
itemLabel (:330): font/color |
🔴 Principle 4 |
-select-all |
selectAllWrapper (:284) + selectAllSizeStyles (:379): display/gap/cursor + padding |
🔴 no paint of its own |
-select-all-divider |
styles.divider (:297): marginBlock |
🔴 Principle 5 |
1. -option-checkbox (MultiSelector.tsx:1286) — wrapper, not the thing that
paints. The rule at the head of Class targeting via themeProps is explicit:
not on "elements that exist only for positioning/event handling." This div is
inert with pointerEvents: none — that is its job. Principle 1 names this
exact case ("if a decorative glyph is wrapped in an inert/positioning div, the
target belongs on the glyph").
There's a second rule I want to make explicit here: when the element you want
to theme is itself an Astryx component composed inside another, attach the class
to that component through its className passthrough — never to a wrapper
minted to carry it — and name it {parent}-{position}-{component}, with
position from a fixed vocabulary (trigger, option, item, row, header,
leading, trailing). Here that's satisfiable with no wrapper at all:
CheckboxInput already takes className (CheckboxInput.tsx:410) and merges it
alongside its own themeProps at :485. The inert div stays, purely for
behavior. Your own merged #4627 is the pattern — selector-check went on the
painted <Icon>.
Worth noting the stated intent (resize the checkbox) isn't actually solved by
either placement: the row checkbox's size derives from the component's size
prop at :1298, and the painted box already has astryx-checkbox with
visualProps: ['size']. Sizing it independently is a structural change, not a
theme override.
2. -option-label (:1304) — Principle 4 rules this one out by name: "a
dedicated -label target that only wraps the fallback span is both redundant and
inconsistent (it silently misses custom-rendered content)." Your test at
:1940 demonstrates exactly that hole. The intent (row label weight) should ride
the row target and inherit.
One honest wrinkle: it won't inherit as-is, because styles.itemLabel (:330)
declares fontFamily/fontSize/fontWeight/color on the span, so a theme's
.astryx-multi-selector-option { font-weight: … } loses to the span's own
declaration. The Principle-4-correct fix is to move that inheritable typography
up onto styles.item so one row target reaches both the fallback label and
renderOption output. Bigger than deleting a class, but it's the change that
makes the row target actually work.
3. -select-all-divider (:1349) — Principle 5. "Do not add a target whose
main purpose is to remove or hide an internal element — that is a signal the
element's presence is itself a design question, not a theming one." The Why and
the test (display: none, :2006) both name hiding as the goal. This divider is
inserted by the component, not declared by the consumer, so it isn't the same
thing as dropdown-menu-divider (which covers {type: 'divider'} items the
consumer passed). If themes don't want a rule under Select all, let's settle that
in the design once (Principle 7) rather than ship an escape hatch. It already
carries astryx-divider in any case.
4. -select-all (:1272) — this reads as a state of the option row, not a
sibling target. The PR itself says so by stamping both classes, and its only
delta over a normal row is padding and display/gap/cursor. Principle 2's shape:
themeProps('multi-selector-option', {
size,
selectAll: isSelectAll || null,
selected: isSelected || null,
disabled: item.disabled || null,
})→ .astryx-multi-selector-option.select-all, documented as
states: ['select-all', 'selected', 'disabled'], visualProps: ['size']. That
also removes the nested mergeProps(themeProps(), themeProps()).
5. -option needs its state and size data (:1274, doc :32). Principle 2:
"an option that can be selected and sized must carry selected and size in its
themeProps, or a theme cannot express 'selected option at large.'" The row is
sized (:1280), disabled (:1283) and selected (:1252), but the target
carries none of it. Heads-up that CI can't catch this — themingTargets.test.ts
is subset-only by design, so an under-declared target stays green.
dropdown-menu-radio is the shape to copy. selector-option has the same gap;
converging both would be a good follow-up.
Cross-PR. #4626 and #4756 make the same wrapper/layout choice — #4626 puts
-indicator on the chevron wrapper while -indicator-icon is already on the
glyph, with transform: none as the intent; #4756's example is max-height +
padding, which is the derived-var pipeline's job. Also flagging that #4628 and
#4626 conflict in all three MultiSelector files (same theming.targets array,
same end-of-file describe anchor) — whichever lands first, the other rebases.
Rather than iterate on four PRs separately, I'd like to settle paint-vs-layout
and the compositional naming rule once and write them into Theming
Infrastructure; happy to do that and then re-review this batch against it.
Where I land: keep -option (with its states/size), drop -option-label in
favor of moving the row's typography up, move -option-checkbox onto
CheckboxInput if it survives the paint-vs-layout test at all, fold -select-all
into a state, and drop -select-all-divider. That's a one-target PR plus a small
typography move — and I think it's the version that ages well.
Nits: description table still lists the pre-rename class names; the Divider gets
themeProps(...).className rather than the spread (:1352), which would drop
data-* if states are ever added; and --color-background-subtle (test :2004)
isn't a token — you want --color-background-muted.
Principle 4 was the one principle with no mechanical check, and its own worked example — "a dedicated -label target that only wraps the fallback span ... silently misses custom-rendered content" — turns out to be detectable without any guess about design intent. - targetOnRenderPropFallback: a target on the element a `render*` callback renders in place of. That is principle 4's named anti-pattern; the callback's output never carries the target. - inheritableOnRenderPropFallback: inheritable typography/color on such a fallback, where hoisting it to the row target covers both paths. - inheritablePropertyOnChild (opt-in, checkInheritableHoisting): the broad form — any inheritable property on an untargeted descendant of a target. 111 hits, most of them correct code (a Banner title and description SHOULD differ), so it stays off as an exploration aid. The narrow render-prop form is 0 on main and fires on #4628. The render-prop predicate is intent-independent by construction: the divergence between the two render paths is in the AST.
| }, | ||
| {className: 'astryx-multi-selector-option'}, | ||
| {className: 'astryx-multi-selector-option-checkbox'}, | ||
| {className: 'astryx-multi-selector-option-label'}, |
There was a problem hiding this comment.
Let's move font styling onto the option so it can be themeable there.
There was a problem hiding this comment.
Done — removed the separate option-label target. Font styling is now themeable via the multi-selector-option target directly through the cascade.
9122190 to
e257e49
Compare
| <div | ||
| inert | ||
| {...mergeProps( | ||
| themeProps('multi-selector-option-checkbox'), |
There was a problem hiding this comment.
Checkbox indicators are directly themeable via #4712 now, so we likely don't want to expose a target on the wrapper. We could also theme the checkbox input below instead.
Since you're out this week, I'll clean this up and get the rest of this merged.
…argets Expose theme targets for the dropdown option rows, per-row checkbox, Select All row, and Select All divider. Font styling is themeable via the option target directly (no separate label target needed). The option target is intended for visual overrides (borderRadius, backgroundColor, padding) — the layout properties (flex, gap) are internal structure and should not be themed.
Review cleanup on facebook#4628, applying the paint-vs-layout frame: - multi-selector-option is the only new target, and now carries the row's size plus its select-all / selected / disabled states, so a theme can express "selected option at large" or restyle just the Select All row. - Row typography moves from the label span onto the row, so a theme's font override on the row target actually reaches the text — and reaches renderOption content too, not just the fallback span. - Drops -option-checkbox: the wrapper is inert/layout-only, and the checkbox visual is directly themeable via checkbox-indicator (facebook#4712). - Drops -select-all (now a state) and -select-all-divider (a target whose purpose is hiding an internal element is a design question, not theming).
e257e49 to
2950818
Compare
|
@cixzhang is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
the
needs:design-review
cixzhang
changed the title
github-actions
Bot
removed
the
needs:code-review
Principle 4 was the one principle with no mechanical check, and its own worked example — "a dedicated -label target that only wraps the fallback span ... silently misses custom-rendered content" — turns out to be detectable without any guess about design intent. - targetOnRenderPropFallback: a target on the element a `render*` callback renders in place of. That is principle 4's named anti-pattern; the callback's output never carries the target. - inheritableOnRenderPropFallback: inheritable typography/color on such a fallback, where hoisting it to the row target covers both paths. - inheritablePropertyOnChild (opt-in, checkInheritableHoisting): the broad form — any inheritable property on an untargeted descendant of a target. 111 hits, most of them correct code (a Banner title and description SHOULD differ), so it stays off as an exploration aid. The narrow render-prop form is 0 on main and fires on #4628. The render-prop predicate is intent-independent by construction: the divergence between the two render paths is in the AST.
Principle 4 was the one principle with no mechanical check, and its own worked example — "a dedicated -label target that only wraps the fallback span ... silently misses custom-rendered content" — turns out to be detectable without any guess about design intent. - targetOnRenderPropFallback: a target on the element a `render*` callback renders in place of. That is principle 4's named anti-pattern; the callback's output never carries the target. - inheritableOnRenderPropFallback: inheritable typography/color on such a fallback, where hoisting it to the row target covers both paths. - inheritablePropertyOnChild (opt-in, checkInheritableHoisting): the broad form — any inheritable property on an untargeted descendant of a target. 111 hits, most of them correct code (a Banner title and description SHOULD differ), so it stays off as an exploration aid. The narrow render-prop form is 0 on main and fires on #4628. The render-prop predicate is intent-independent by construction: the divergence between the two render paths is in the AST.
2 participants
What
Dropdown option rows in
MultiSelectorare themeable through one stable target—
astryx-multi-selector-option— carrying the row'ssizeand itsselect-all,selectedanddisabledstates:Row typography moves from the label span up onto the row (matching
Selector,whose option row already owns its typography). With the font declared on the
span, a theme's
font-weighton the row always lost to it; on the row itreaches both the fallback label and
renderOptioncontent.Why
The option rows had no theme slot, unlike single-select
Selector, sorestyling a row meant structural selectors against internal StyleX class names.
The PR opened with five targets; four of them sat on layout-only elements or
exposed internal structure, and are gone:
-option-checkbox— aninert,pointer-events: nonepositioning wrapper.The checkbox visual itself is directly themeable as
checkbox-indicatorsince feat(theme): add themeable indicators #4712, which is the element that actually paints.
-select-all— reads as a state of the option row, not a sibling target;it is now
.astryx-multi-selector-option.select-all.-select-all-divider— its stated purpose was hiding an internal element,which is a design question rather than a theming one. It already carries
astryx-divider.-option-label— redundant with the row target, and it silently missedcustom-rendered rows. The typography move covers the intent instead.
Risk
Default rows are unchanged, pixel for pixel. One deliberate visual change:
content rendered by
renderOptionnow inherits the row's font and disabledcolor where it previously inherited from the page — that inheritance is what
lets a single row target reach custom rows.
Testing
main(feat(selector): add disabled-trigger and caret-wrapper theme targets #4626 landed in the same three files).the theming-targets guard — 825 tests green.
disabled rows diff by 0 pixels;
renderOptionrows differ only in the labeltext area (0.66% of the frame), as intended.