From 374730865e813822e3d4e42941b89c61217d3bd8 Mon Sep 17 00:00:00 2001 From: cixzhang Date: Wed, 24 Jun 2026 23:18:13 +0000 Subject: [PATCH] fix(docsite): playground number knobs show unset, not a fake 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Optional numeric props (e.g. Card's maxWidth, width, height, minHeight) have no default, so the playground state leaves them unset and the live component renders without that style. The number control, however, fell back to displaying `0` for any non-number value — misrepresenting the real state and, worse, giving no way back to "unset". Stepping such a prop 0 -> 1 -> 0 wrote a literal value into the source (e.g. maxWidth={0}), collapsing sizing props with no path back to the original render. Make the number control honest about unset state in both playground surfaces (the component detail Properties tab and the /playground popover): show an empty input with an "unset" placeholder when the prop has no value, and add a clear button so users can return to unset. Clearing now removes the attribute instead of writing a literal. Required numeric props keep their generated fallback and stay non-clearable. Adds regression coverage for the SizeValue/number control mapping and the clear-to-unset source round-trip. --- .../playground-number-control.test.ts | 80 +++++++++++++++++++ .../propertyEditor/PropertyEditor.tsx | 66 +++++++++------ .../component-detail/PlaygroundPropsTable.tsx | 11 ++- 3 files changed, 132 insertions(+), 25 deletions(-) create mode 100644 apps/docsite/src/__tests__/playground-number-control.test.ts diff --git a/apps/docsite/src/__tests__/playground-number-control.test.ts b/apps/docsite/src/__tests__/playground-number-control.test.ts new file mode 100644 index 000000000000..e8a20eceee11 --- /dev/null +++ b/apps/docsite/src/__tests__/playground-number-control.test.ts @@ -0,0 +1,80 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file playground-number-control.test.ts + * + * Regression coverage for the playground "number" knob (issue: a Card with an + * unset \`maxWidth\` showed \`0\` in the editor, and stepping 0 -> 1 -> 0 left a + * literal \`maxWidth={0}\` in the source — collapsing sizing props with no path + * back to the original unset render). + * + * Two invariants are pinned here: + * 1. \`SizeValue\` (and bare \`number\`) resolve to the \`number\` control, so size + * props get a numeric knob in the first place. + * 2. Clearing a numeric prop in the Properties popover removes the attribute + * from source (returning it to unset) instead of writing a literal value. + * This mirrors the \`commit('number', null)\` branch in PropertyEditor. + */ + +import {describe, expect, it} from 'vitest'; +import {parsePropType} from '../components/component-detail/parsePropType'; +import { + analyzeCode, + formatAttr, + removeAttribute, + setAttribute, + type InstanceInfo, +} from '../app/playground/propertyEditor/componentInstances'; + +function firstInstance(code: string): InstanceInfo { + const instances = analyzeCode(code); + expect(instances).not.toBeNull(); + expect(instances!.length).toBeGreaterThan(0); + return instances![0]; +} + +/** + * Mirror of PropertyEditor's number commit handler: a null value clears the + * prop, any concrete number writes a literal. + */ +function commitNumber( + code: string, + instance: InstanceInfo, + name: string, + value: number | null, +): string { + return value == null + ? removeAttribute(code, instance, name) + : setAttribute(code, instance, name, formatAttr(name, 'number', value)); +} + +describe('playground number control', () => { + it('maps SizeValue and number to the number control', () => { + expect(parsePropType('SizeValue', 'maxWidth')).toEqual({kind: 'number'}); + expect(parsePropType('number', 'ratio')).toEqual({kind: 'number'}); + }); + + it('clearing a numeric prop removes it from source (no literal left behind)', () => { + const base = 'content'; + + // Step the value up to 1, as the stepper would. + const withMaxWidth = commitNumber(base, firstInstance(base), 'maxWidth', 1); + expect(withMaxWidth).toBe('content'); + + // Clearing returns the prop to unset rather than writing maxWidth={0}. + const cleared = commitNumber( + withMaxWidth, + firstInstance(withMaxWidth), + 'maxWidth', + null, + ); + expect(cleared).toBe(base); + expect(cleared).not.toContain('maxWidth'); + }); + + it('explicitly setting 0 still writes a literal (0 is a valid value)', () => { + const base = 'content'; + const withZero = commitNumber(base, firstInstance(base), 'maxWidth', 0); + expect(withZero).toBe('content'); + }); +}); diff --git a/apps/docsite/src/app/playground/propertyEditor/PropertyEditor.tsx b/apps/docsite/src/app/playground/propertyEditor/PropertyEditor.tsx index a20a4320c7d0..9defc0a18056 100644 --- a/apps/docsite/src/app/playground/propertyEditor/PropertyEditor.tsx +++ b/apps/docsite/src/app/playground/propertyEditor/PropertyEditor.tsx @@ -113,7 +113,7 @@ function PropRow({prop, instance, code, onCodeChange}: PropRowProps) { const commit = ( kind: 'boolean' | 'string' | 'number' | 'enum', - value: string | number | boolean, + value: string | number | boolean | null, ) => { let next: string; if (kind === 'boolean') { @@ -133,30 +133,45 @@ function PropRow({prop, instance, code, onCodeChange}: PropRowProps) { code, instance, prop.name, - formatAttr(prop.name, 'string', value), + formatAttr(prop.name, 'string', String(value)), ); } else if (kind === 'number') { - next = setAttribute( - code, - instance, - prop.name, - formatAttr(prop.name, 'number', value), - ); + // A cleared number returns the prop to unset rather than writing a + // literal (e.g. maxWidth={0}, which collapses sizing props). + next = + value == null + ? removeAttribute(code, instance, prop.name) + : setAttribute( + code, + instance, + prop.name, + formatAttr(prop.name, 'number', value), + ); } else { - const numeric = - typeof value === 'number' || NUMERIC_RE.test(String(value)); - const attrKind = - typeof value === 'boolean' ? 'boolean' : numeric ? 'number' : 'string'; - next = setAttribute( - code, - instance, - prop.name, - formatAttr( + // Enum commits always carry a concrete option value; treat a null (only + // emitted by the number control) as clearing the attribute. + if (value == null) { + next = removeAttribute(code, instance, prop.name); + } else { + const numeric = + typeof value === 'number' || NUMERIC_RE.test(String(value)); + const attrKind = + typeof value === 'boolean' + ? 'boolean' + : numeric + ? 'number' + : 'string'; + next = setAttribute( + code, + instance, prop.name, - attrKind, - numeric && typeof value !== 'number' ? Number(value) : value, - ), - ); + formatAttr( + prop.name, + attrKind, + numeric && typeof value !== 'number' ? Number(value) : value, + ), + ); + } } onCodeChange(next); }; @@ -208,13 +223,18 @@ function PropRow({prop, instance, code, onCodeChange}: PropRowProps) { ); } else { const def = coerceDefault(prop.default, control) as number | undefined; - const value = typeof attr?.value === 'number' ? attr.value : (def ?? 0); + // Reflect the actual source state: a prop that isn't set in code (and has + // no default) is unset, not `0`. Showing a real `0` here would let a size + // prop like maxWidth render `max-width: 0` with no way back to unset. + const value = typeof attr?.value === 'number' ? attr.value : (def ?? null); controlEl = ( commit('number', next)} + placeholder={prop.required ? undefined : 'unset'} + hasClear={!prop.required} + onChange={(next: number | null) => commit('number', next)} xstyle={s.inputControl} /> ); diff --git a/apps/docsite/src/components/component-detail/PlaygroundPropsTable.tsx b/apps/docsite/src/components/component-detail/PlaygroundPropsTable.tsx index da87d562ab6b..35a246b26eb8 100644 --- a/apps/docsite/src/components/component-detail/PlaygroundPropsTable.tsx +++ b/apps/docsite/src/components/component-detail/PlaygroundPropsTable.tsx @@ -429,11 +429,18 @@ function InlineControl({ /> ); case 'number': + // An optional prop with no default is genuinely unset — show an empty + // input (not a fabricated `0`, which misrepresents the live state and, + // for size props like maxWidth, has no path back to "unset"). `hasClear` + // lets the user return to the original unset render. Required numbers + // keep their generated fallback and stay non-clearable. return ( onChange(next)} + value={typeof value === 'number' ? value : null} + placeholder={prop.required ? undefined : 'unset'} + hasClear={!prop.required} + onChange={(next: number | null) => onChange(next ?? undefined)} /> ); case 'element':