diff --git a/.changeset/fix-indicator-falsy-children.md b/.changeset/fix-indicator-falsy-children.md new file mode 100644 index 000000000000..06762727ce57 --- /dev/null +++ b/.changeset/fix-indicator-falsy-children.md @@ -0,0 +1,11 @@ +--- +'@astryxdesign/core': patch +--- + +[fix] Indicator: a falsy `children` no longer deletes the state mark. The busy idiom a host actually writes — `children={isBusy && }` — passes `false` when it is not busy, and `false` is neither `null` nor caught by `??`, so all three indicators took the children path, rendered nothing in it, and dropped the checkmark, the checkbox tick and the radio dot on every selected row. They now use `isRenderable`, so only children that actually render replace the mark. `0` still counts as content, since it renders the character "0". + +CheckIndicator's children slot also reserves the glyph's box and carries its color, so swapping a Spinner in no longer shifts the row or loses the disabled shade. + +Fixes #4893. + +@cixzhang diff --git a/apps/storybook/stories/Indicator.stories.tsx b/apps/storybook/stories/Indicator.stories.tsx new file mode 100644 index 000000000000..45630bddca5c --- /dev/null +++ b/apps/storybook/stories/Indicator.stories.tsx @@ -0,0 +1,254 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +import type {Meta, StoryObj} from '@storybook/react'; +import {useState, type ReactNode} from 'react'; +import { + CheckboxIndicator, + CheckIndicator, + RadioIndicator, +} from '@astryxdesign/core/Indicator'; +import {Spinner} from '@astryxdesign/core/Spinner'; +import {Text} from '@astryxdesign/core/Text'; +import {HStack, VStack} from '@astryxdesign/core/Stack'; + +/** + * Indicators are the componentized selection visuals shared by CheckboxInput, + * RadioList, Selector and menu selection rows. They are decorative: the owning + * component keeps the input, role, accessible name, focus and keyboard + * behavior, while the indicator turns `state` into a picture. + * + * These stories render them directly, which no other story file does — every + * other one reaches an indicator through a host. That matters for the + * `children` slot in particular: it is the path a host uses to show a pending + * Spinner, and it had no rendered coverage anywhere until this file existed. + */ +const meta: Meta = { + title: 'Core/Indicator', + component: CheckIndicator, + parameters: {layout: 'padded'}, + tags: ['autodocs'], +}; + +export default meta; +// Keyed to the component, not to `meta`, matching the other 122 story files — +// `StoryObj` would require `args` on every render-only story, and +// `state` is required on an indicator. +type Story = StoryObj; + +const cellStyle = { + display: 'inline-flex', + alignItems: 'center', + justifyContent: 'center', + minWidth: 40, + minHeight: 32, +} as const; + +function Row({label, children}: {label: string; children: ReactNode}) { + return ( + + + + {label} + + + {children} + + ); +} + +/** Every indicator, in every state its family defines. */ +export const AllStates: Story = { + render: () => ( + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + ), +}; + +/** Both control sizes, side by side. */ +export const Sizes: Story = { + render: () => ( + + + + + + + + + + + + + + + + + + + + + + + + + ), +}; + +/** Disabled is purely visual — the owner keeps the real disabled semantics. */ +export const Disabled: Story = { + render: () => ( + + + + + + + + + + + + + + + + + + + + + + + + + ), +}; + +/** + * The `children` slot, and the reason it needs a story. + * + * A host shows a pending change by passing a Spinner through `children`, and + * the idiom it writes is `children={isBusy && }`. When `isBusy` is + * false that passes `false` — which is neither `null` nor caught by `??`. Every + * indicator used to take the children path on it, render nothing there, and + * DELETE its state mark (#4893). + * + * Read the first column: each cell must still show its mark. Only the second + * column should show a spinner in place of one. + */ +export const BusyChildren: Story = { + render: () => { + const idiom = (isBusy: boolean) => + isBusy && ; + + return ( + + + Column 1 keeps its mark (children renders nothing). Column 2 shows the + spinner instead of the mark. + + + + {idiom(false)} + + + {idiom(true)} + + + + + + {idiom(false)} + + + + {idiom(true)} + + + + + + {idiom(false)} + + + + + {idiom(true)} + + + + + + {idiom(false)} + + + {idiom(true)} + + + + ); + }, +}; + +/** + * A live toggle of the same idiom: flip busy and the spinner replaces the mark, + * flip it back and the mark returns. Before #4893 the mark did not come back — + * it never rendered in the first place. + */ +export const BusyToggle: Story = { + render: function BusyToggleStory() { + const [isBusy, setIsBusy] = useState(false); + const busy = isBusy && ; + + return ( + + + + + {busy} + + + {busy} + + + {busy} + + + + ); + }, +}; diff --git a/packages/cli/authoring/doctypes/base/type.ts b/packages/cli/authoring/doctypes/base/type.ts index 93a151c097c4..5275c8097d84 100644 --- a/packages/cli/authoring/doctypes/base/type.ts +++ b/packages/cli/authoring/doctypes/base/type.ts @@ -260,6 +260,8 @@ export interface ComponentSlotElement { * { property: 'padding', vars: ['--_card-padding'] }, * ``` */ +// SYNC: apps/docsite/scripts/generate-data.mjs (interface DerivedVar) — see the +// note on ComponentThemingTarget below. export interface ComponentThemingDerivedVar { /** The standard CSS property name (camelCase) that theme authors write. * e.g. `'borderRadius'`, `'padding'`, `'paddingBlock'` */ @@ -291,6 +293,10 @@ export interface ComponentThemingDerivedVar { * {className: 'astryx-card'} * ``` */ +// SYNC: When adding a field here, add it to the docsite's generated-registry +// copy too — apps/docsite/scripts/generate-data.mjs (interface ThemingTarget). +// `next build` type-checks the emitted componentRegistry.ts against that copy, +// so a field present in a .doc.mjs but missing there fails the docsite build. export interface ComponentThemingTarget { /** The stable CSS class name rendered by the component. * Always starts with `astryx-`. diff --git a/packages/core/src/Indicator/CheckIndicator.tsx b/packages/core/src/Indicator/CheckIndicator.tsx index a9358b014237..b68042d6a7d4 100644 --- a/packages/core/src/Indicator/CheckIndicator.tsx +++ b/packages/core/src/Indicator/CheckIndicator.tsx @@ -32,7 +32,8 @@ import * as stylex from '@stylexjs/stylex'; import type {SVGProps} from 'react'; import {Icon} from '../Icon/Icon'; -import {mergeProps} from '../utils'; +import {colorVars} from '../theme/tokens.stylex'; +import {isRenderable, mergeProps} from '../utils'; import type {IndicatorProps} from './types'; /** The check glyph matches the control sizes the indicator families share. */ @@ -41,6 +42,27 @@ const iconSizeForIndicator = { md: 'sm', } as const; +const styles = stylex.create({ + // The children slot stands where the glyph would, so swapping a Spinner in + // for the mark does not move the row. 1rem is Icon's `sm` box, which is what + // `iconSizeForIndicator` resolves to at both indicator sizes. + slot: { + display: 'inline-flex', + alignItems: 'center', + justifyContent: 'center', + flexShrink: 0, + width: '1rem', + height: '1rem', + }, + // Foreground for an inherit-shade Spinner, matching what the glyph would + // have painted. The sibling indicators set `color` on their chrome for the + // same reason. + enabled: {color: colorVars['--color-accent']}, + // The same token Icon's `color="disabled"` resolves to, so the busy and the + // glyph paths read identically when the owner is disabled. + disabled: {color: colorVars['--color-icon-disabled']}, +}); + /** * The default single-selection mark: a checkmark when chosen, nothing when not. * @@ -79,10 +101,15 @@ export function CheckIndicator({ // BEFORE `state`: a host passes a busy visual through in whatever state the // row happens to be in, and an unchecked listbox row is the common one. // + // `isRenderable`, not `children != null`: the idiom a host actually writes is + // `children={isBusy && }`, which passes `false` when it is not + // busy. `false` is non-null, so a null check takes this branch, renders + // nothing inside it, and DELETES the mark on a chosen row (#4893). + // // There is no glyph to hang the caller's props on in this branch, so they go // on a span — every one of them, so a `data-testid`, an `id`, a handler or // an `xstyle` behaves the same whether or not children are present. - if (children != null) { + if (isRenderable(children)) { return ( ); diff --git a/packages/core/src/Indicator/CheckboxIndicator.tsx b/packages/core/src/Indicator/CheckboxIndicator.tsx index 5e2a1050362e..06e7e3ab34b3 100644 --- a/packages/core/src/Indicator/CheckboxIndicator.tsx +++ b/packages/core/src/Indicator/CheckboxIndicator.tsx @@ -16,7 +16,7 @@ import { easeVars, radiusVars, } from '../theme/tokens.stylex'; -import {mergeProps, themeProps} from '../utils'; +import {isRenderable, mergeProps, themeProps} from '../utils'; import {indicatorScope} from './indicator.markers.stylex'; import type {IndicatorProps} from './types'; @@ -217,7 +217,9 @@ export function CheckboxIndicator({ style, )} {...rest}> - {children ?? ( + {isRenderable(children) ? ( + children + ) : ( <> { }); }); +/** + * The busy idiom a host actually writes is `children={isBusy && }`, + * which passes `false` when it is not busy. `false` is non-null and is not + * caught by `??`, so both a `!= null` guard and a `children ?? mark` fallback + * take the children path, render nothing in it, and delete the state mark + * (#4893). Every indicator gets the same case, because all three had the bug. + */ +describe('falsy children never suppress the state mark (#4893)', () => { + const cases = [ + { + name: 'CheckIndicator', + render: (children: ReactNode) => + render({children}), + markSelector: '.astryx-icon', + }, + { + name: 'CheckboxIndicator', + render: (children: ReactNode) => + render( + {children}, + ), + markSelector: 'svg', + }, + { + name: 'RadioIndicator', + render: (children: ReactNode) => + render({children}), + markSelector: '.astryx-radio-indicator-dot', + }, + ] as const; + + // Everything React renders as nothing. `0` is deliberately absent: it + // renders the visible text "0", so it IS content and must replace the mark. + const emptyValues = [ + ['false — the `isBusy && …` idiom', false], + ['null', null], + ['undefined', undefined], + ['empty string', ''], + ] as const; + + for (const {name, render: renderCase, markSelector} of cases) { + for (const [label, child] of emptyValues) { + it(`${name} keeps its mark when children is ${label}`, () => { + const {container} = renderCase(child); + + expect( + container.querySelector(markSelector), + `${name} lost its mark to a falsy child`, + ).toBeInTheDocument(); + }); + } + + it(`${name} still lets real children replace the mark`, () => { + // The negative control: without this, "always render the mark" would + // pass every case above and break the busy state instead. + const {container} = renderCase(); + + expect( + container.querySelector('[data-testid="busy"]'), + ).toBeInTheDocument(); + expect(container.querySelector(markSelector)).not.toBeInTheDocument(); + }); + } + + it('treats 0 as content, not as empty', () => { + // isRenderable's documented edge: 0 renders the character "0". + const {container} = render( + {0}, + ); + + expect(container.textContent).toBe('0'); + expect(container.querySelector('.astryx-icon')).not.toBeInTheDocument(); + }); +}); + describe('useIndicator', () => { it('returns the built-in indicator without a theme override', () => { const {result} = renderHook(() => useIndicator('checkbox')); diff --git a/packages/core/src/Indicator/RadioIndicator.tsx b/packages/core/src/Indicator/RadioIndicator.tsx index 426552011ee2..d48315dd0fa2 100644 --- a/packages/core/src/Indicator/RadioIndicator.tsx +++ b/packages/core/src/Indicator/RadioIndicator.tsx @@ -16,7 +16,7 @@ import { easeVars, radiusVars, } from '../theme/tokens.stylex'; -import {mergeProps, themeProps} from '../utils'; +import {isRenderable, mergeProps, themeProps} from '../utils'; import {indicatorScope} from './indicator.markers.stylex'; import type {IndicatorProps} from './types'; @@ -181,19 +181,20 @@ export function RadioIndicator({ style, )} {...rest}> - {children ?? - (isChecked && ( - - ))} + {isRenderable(children) + ? children + : isChecked && ( + + )} ); }