fix(indicator): a falsy child no longer deletes the state mark (#4893)

.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 && <Spinner/>}` — 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

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<typeof CheckIndicator> = {
+  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<typeof meta>` would require `args` on every render-only story, and
+// `state` is required on an indicator.
+type Story = StoryObj<typeof CheckIndicator>;
+
+const cellStyle = {
+  display: 'inline-flex',
+  alignItems: 'center',
+  justifyContent: 'center',
+  minWidth: 40,
+  minHeight: 32,
+} as const;
+
+function Row({label, children}: {label: string; children: ReactNode}) {
+  return (
+    <HStack gap={4} vAlign="center">
+      <span style={{minWidth: 260}}>
+        <Text type="supporting" color="secondary">
+          {label}
+        </Text>
+      </span>
+      {children}
+    </HStack>
+  );
+}
+
+/** Every indicator, in every state its family defines. */
+export const AllStates: Story = {
+  render: () => (
+    <VStack gap={4}>
+      <Row label="CheckIndicator — unchecked, checked">
+        <span style={cellStyle}>
+          <CheckIndicator state="unchecked" />
+        </span>
+        <span style={cellStyle}>
+          <CheckIndicator state="checked" />
+        </span>
+      </Row>
+      <Row label="CheckboxIndicator — unchecked, checked, indeterminate">
+        <span style={cellStyle}>
+          <CheckboxIndicator state="unchecked" />
+        </span>
+        <span style={cellStyle}>
+          <CheckboxIndicator state="checked" />
+        </span>
+        <span style={cellStyle}>
+          <CheckboxIndicator state="indeterminate" />
+        </span>
+      </Row>
+      <Row label="RadioIndicator — unchecked, checked">
+        <span style={cellStyle}>
+          <RadioIndicator state="unchecked" />
+        </span>
+        <span style={cellStyle}>
+          <RadioIndicator state="checked" />
+        </span>
+      </Row>
+    </VStack>
+  ),
+};
+
+/** Both control sizes, side by side. */
+export const Sizes: Story = {
+  render: () => (
+    <VStack gap={4}>
+      <Row label="sm">
+        <span style={cellStyle}>
+          <CheckIndicator state="checked" size="sm" />
+        </span>
+        <span style={cellStyle}>
+          <CheckboxIndicator state="checked" size="sm" />
+        </span>
+        <span style={cellStyle}>
+          <RadioIndicator state="checked" size="sm" />
+        </span>
+      </Row>
+      <Row label="md (default)">
+        <span style={cellStyle}>
+          <CheckIndicator state="checked" size="md" />
+        </span>
+        <span style={cellStyle}>
+          <CheckboxIndicator state="checked" size="md" />
+        </span>
+        <span style={cellStyle}>
+          <RadioIndicator state="checked" size="md" />
+        </span>
+      </Row>
+    </VStack>
+  ),
+};
+
+/** Disabled is purely visual — the owner keeps the real disabled semantics. */
+export const Disabled: Story = {
+  render: () => (
+    <VStack gap={4}>
+      <Row label="disabled — unchecked">
+        <span style={cellStyle}>
+          <CheckIndicator state="unchecked" isDisabled />
+        </span>
+        <span style={cellStyle}>
+          <CheckboxIndicator state="unchecked" isDisabled />
+        </span>
+        <span style={cellStyle}>
+          <RadioIndicator state="unchecked" isDisabled />
+        </span>
+      </Row>
+      <Row label="disabled — checked">
+        <span style={cellStyle}>
+          <CheckIndicator state="checked" isDisabled />
+        </span>
+        <span style={cellStyle}>
+          <CheckboxIndicator state="checked" isDisabled />
+        </span>
+        <span style={cellStyle}>
+          <RadioIndicator state="checked" isDisabled />
+        </span>
+      </Row>
+    </VStack>
+  ),
+};
+
+/**
+ * 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 && <Spinner/>}`. 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 && <Spinner size="sm" shade="inherit" />;
+
+    return (
+      <VStack gap={4}>
+        <Text type="supporting" color="secondary">
+          Column 1 keeps its mark (children renders nothing). Column 2 shows the
+          spinner instead of the mark.
+        </Text>
+        <Row label="CheckIndicator — checked">
+          <span style={cellStyle}>
+            <CheckIndicator state="checked">{idiom(false)}</CheckIndicator>
+          </span>
+          <span style={cellStyle}>
+            <CheckIndicator state="checked">{idiom(true)}</CheckIndicator>
+          </span>
+        </Row>
+        <Row label="CheckboxIndicator — checked">
+          <span style={cellStyle}>
+            <CheckboxIndicator state="checked">
+              {idiom(false)}
+            </CheckboxIndicator>
+          </span>
+          <span style={cellStyle}>
+            <CheckboxIndicator state="checked">{idiom(true)}</CheckboxIndicator>
+          </span>
+        </Row>
+        <Row label="CheckboxIndicator — indeterminate">
+          <span style={cellStyle}>
+            <CheckboxIndicator state="indeterminate">
+              {idiom(false)}
+            </CheckboxIndicator>
+          </span>
+          <span style={cellStyle}>
+            <CheckboxIndicator state="indeterminate">
+              {idiom(true)}
+            </CheckboxIndicator>
+          </span>
+        </Row>
+        <Row label="RadioIndicator — checked">
+          <span style={cellStyle}>
+            <RadioIndicator state="checked">{idiom(false)}</RadioIndicator>
+          </span>
+          <span style={cellStyle}>
+            <RadioIndicator state="checked">{idiom(true)}</RadioIndicator>
+          </span>
+        </Row>
+      </VStack>
+    );
+  },
+};
+
+/**
+ * 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 && <Spinner size="sm" shade="inherit" />;
+
+    return (
+      <VStack gap={4}>
+        <label>
+          <input
+            type="checkbox"
+            checked={isBusy}
+            onChange={e => setIsBusy(e.target.checked)}
+          />{' '}
+          <Text type="supporting">isBusy</Text>
+        </label>
+        <HStack gap={4} vAlign="center">
+          <span style={cellStyle}>
+            <CheckIndicator state="checked">{busy}</CheckIndicator>
+          </span>
+          <span style={cellStyle}>
+            <CheckboxIndicator state="checked">{busy}</CheckboxIndicator>
+          </span>
+          <span style={cellStyle}>
+            <RadioIndicator state="checked">{busy}</RadioIndicator>
+          </span>
+        </HStack>
+      </VStack>
+    );
+  },
+};

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-`.

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 && <Spinner/>}`, 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 (
       <span
         // Spread first, as on the glyph path below, so the indicator's own
@@ -91,7 +118,15 @@ export function CheckIndicator({
         {...rest}
         ref={ref}
         aria-hidden="true"
-        {...mergeProps(stylex.props(xstyle), className, style)}>
+        {...mergeProps(
+          stylex.props(
+            styles.slot,
+            isDisabled ? styles.disabled : styles.enabled,
+            xstyle,
+          ),
+          className,
+          style,
+        )}>
         {children}
       </span>
     );

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
+      ) : (
         <>
           <svg
             viewBox="0 0 10 10"