feat: adopt t-shirt size prop across components - #796
Conversation
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
…ning all linear issues
Add xs/s/m/l size prop (default l) to web + mobile Button, deprecate compact at v10 with size wins over compact, plus stories, docs, and Figma Code Connect updates. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Add xs/s/m/l size prop to web and mobile IconButton, deprecate compact at v10, refresh stories/docs/Figma Code Connect while preserving byte-for-byte backward compatibility. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Plumb size (s/m/l) through SearchInput to TextInput on web + mobile, deprecate compact, and remove web's fixed-height computation so height stays content-derived. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Add size (s/m/l) to the alpha Select on web + mobile, resolving compact to size='s' for backward compatibility, deprecate compact for v10, and refresh stories, docs, and Figma Code Connect. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Add s/m/l size prop (default l) routed to the existing fixed-height mechanism, deprecate compact in favor of size="s" at v10, and refresh docs, stories, and Figma Code Connect. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Thread size (s/m/l, default l) through DatePicker and DateInput on web and mobile, reusing TextInput density, and deprecate compact at v10 (size wins over compact). Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…s (CDS-2224, CDS-2223, CDS-2173) InputChip: inherits size from the shared base Chip; compact deprecated at v10. SelectChip: threads size through the alpha control into MediaChip; compact deprecated at v10. TabbedChips: forwards resolved size to each tab chip; compact deprecated at v10. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
057bb46 to
79bdf5c
Compare
| expect(screen.getByRole('button')).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('renders with size="xs"', () => { |
There was a problem hiding this comment.
test cases that only check if something renders aren't valuable and should be removed, across all unit test files. A more useful test would be to try and measure the derived height to see if it matches the expectation based on the spec in figma. This may not be possible (or easy enough to justify) for components without a forwarded ref or if that ref is assigned the wrong element
| * @deprecated Use `size="xs"` instead. This will be removed in a future major release. | ||
| * @deprecationExpectedRemoval v10 | ||
| */ | ||
| compact?: boolean; |
There was a problem hiding this comment.
do we really need to override compact and size from Chip? Chip should have both these changes and I think the jsdocs there should be generic enough to work in this context - let's rethink this
There was a problem hiding this comment.
apply the same for web as well if you decide it shoudl be changed
| invertColorScheme={invertColorScheme} | ||
| maxWidth={maxWidth} | ||
| numberOfLines={numberOfLines} | ||
| {...props} |
There was a problem hiding this comment.
i dont think we should have moved the props spread to the top, this can result in different behavior than before
| maxWidth, | ||
| displayValue, | ||
| }: SelectControlProps<Type, SelectOptionValue> & | ||
| }: Omit<SelectControlProps<Type, SelectOptionValue>, 'size' | 'compact'> & |
There was a problem hiding this comment.
this feels werid to omit these but i guess it is because they are already coming along for the ride on SelectChipBaseProps - aren't there other props that are on both since i imagine much of the select control's props come from the select itselft
|
|
||
| export type ButtonSize = 'xs' | 's' | 'm' | 'l'; | ||
|
|
||
| type ButtonSizeConfig = { |
There was a problem hiding this comment.
no - this should Pick from the appropriate props type which should be the authoritative source for components. Do not make new type literals for size configurations for all affected components in this PR
|
|
||
| type IconButtonFeedback = 'light' | 'normal'; | ||
|
|
||
| type IconButtonSizeConfig = { |
There was a problem hiding this comment.
no - use the proper, authoritative type for the component props that these are given to as arguments
| * IconButton is a special case: because `compact` defaults to `true`, an IconButton | ||
| * with no `size` (and no `compact={false}`) resolves to `s`, NOT `l`. An explicit | ||
| * `size` always wins over `compact` when both are provided. | ||
| * @default 's' (because `compact` defaults to `true`; resolves to `l` only when `compact={false}`) |
There was a problem hiding this comment.
i think we should document as l being the default as that is what it will be when compact is fully removed
There was a problem hiding this comment.
This is good information but this does read a little bit like a developer note. Keep in mind js docs are extracted as the documenation for our docsite. See if you can reword this while keeping the information
|
|
||
| export type SlideButtonSize = 's' | 'm' | 'l'; | ||
|
|
||
| type SlideButtonSizeConfig = { |
There was a problem hiding this comment.
no - use properties from SlideButtonBasePriops
… padding (web + mobile)
Rework how TextInput derives and applies its density/padding, kept in sync across web and mobile:
- Deliver field padding via InputStack's styles.input (the field container that
surrounds start/label/input/end and sits inside the border) instead of
distributing padding across the input and start/end wrappers. Fixes outer
padding landing on the wrong side of start/end nodes; input now carries none.
- Split the old single density hook into two focused hooks:
- useTextInputPlacement -> returns just the label placement
('outside' | 'inside-horizontal' | 'inside-vertical')
- useTextInputDensity -> returns { contentPadding, contentGap }
Both take already-resolved compact/size; TextInput owns the "size wins over
compact" precedence (consistent with the rest of CDS).
- Collapse the redundant 'legacy-compact' placement into 'inside-horizontal'
driven by the compact flag.
- Padding fixes: legacy compact field height 42px (start-slot label paddingY 0);
vertically-stacked inside label (size l) 58px via symmetric space-0.75.
- NativeInput: deprecate compact/containerSpacing; default padding handled by the
container, input padding reset to 0.
- Update tests + NativeInput story accordingly.
Generated with Claude Code
Co-Authored-By: Claude <noreply@anthropic.com>
The mobile NativeInput set minHeight to the font's lineHeight token but never set lineHeight itself, so input text rendered at the font's natural line height (~19.7px for 16px body) instead of the theme's 24px. Web gets lineHeight from the Box font token, so its input line box is already 24px. This left mobile inputs a few px shorter than web (and design) for the same size, and the minHeight floor only engaged when the input had no padding. Setting lineHeight: theme.lineHeight[font] aligns the mobile input line box with web. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…tainer
We initially applied the whole content-padding box to the InputStack field
container via styles.input (all four edges on the container). That regressed the
field height whenever a start/end adornment is taller than the text line: with
vertical padding on the container, the adornment becomes the tallest child and the
container's top/bottom padding stacks *around* it. SearchInput (a ~44px icon
button) grew from ~58px to ~74px at size l.
So we backed off using styles.input for the *vertical* padding and split the box
by axis:
- Horizontal (left/right) + the inter-slot gap stay on the container via
styles.input — this is what correctly hugs the field edges and separates the
start / input / end slots (and fixed the original "outer padding trapped on the
wrong side of an adornment" bug).
- Vertical (top/bottom) moves back onto the content — the input, and for a
stacked inside-vertical label its top band. The padded input now drives the
field height, so a tall adornment centers within it instead of inflating it.
Also documents the model on ContentPadding, names the vertical-padding values in
TextInput for readability, and updates the affected tests. Web and mobile kept in
sync.
Generated with Claude Code
Co-Authored-By: Claude <noreply@anthropic.com>
…put alignment Strip baked-in padding from InputIcon/InputIconButton so field density owns spacing, and on mobile keep vertical sizing on a wrapper View so RN TextInput does not mis-center glyphs or stretch end icons to the suffix line box. Co-authored-by: Cursor <cursoragent@cursor.com>
…sition Consolidate one-off *Size.stories into each component's main stories, add a cross-control SizeComposition story for row alignment, and fix DateInput helperText="" plus DatePicker popover offset when helper text is suppressed. Co-authored-by: Cursor <cursoragent@cursor.com>
Restore apps/docs examples and __figma__ templates to master so this PR stays focused on component size API + stories. Those follow-ups will land in separate PRs. Co-authored-by: Cursor <cursoragent@cursor.com>
| s: { padding: 1.5, iconSize: 's', borderRadius: 1000 }, | ||
| m: { padding: 1.5, iconSize: 'm', borderRadius: 1000 }, | ||
| l: { padding: 2, iconSize: 'm', borderRadius: 1000 }, | ||
| } as const satisfies Record<IconButtonSize, IconButtonSizeConfig>; |
There was a problem hiding this comment.
dont use a new type IconButtonSizeConfig just Pick properties from the actual props. Apply this to every component who it set up with this pattern
| * explicitly `false`. IconButton defaults `compact` to `true`, so the *effective* default | ||
| * size is `s` — see the `compact`/`size` prop docs and the `resolvedSize` logic below. | ||
| */ | ||
| const defaultIconButtonSize: IconButtonSize = 'l'; |
There was a problem hiding this comment.
let's use this pattern for all sized components - make sure all follow
| // nominal default of the size scale. `compact={false}` yields `defaultIconButtonSize` | ||
| // (`l`). An explicit `size` always wins over `compact` when both are provided. | ||
| const resolvedSize = size ?? (compact ? 's' : defaultIconButtonSize); | ||
| const cfg = iconButtonSizes[resolvedSize]; |
There was a problem hiding this comment.
use descriptive names, not cfg
| import type { ChipProps, ChipSize } from './ChipProps'; | ||
| export type { ChipProps, ChipSize }; | ||
|
|
||
| // values chosen to reproduce today's byte-for-byte top-level Chip defaults |
There was a problem hiding this comment.
you can remove this comment
| ref: React.ForwardedRef<HTMLButtonElement | HTMLDivElement>, | ||
| ) { | ||
| const mergedProps = useComponentConfig('Chip', _props); | ||
| const cfg = chipSizes[mergedProps.size ?? (mergedProps.compact ? 'xs' : defaultChipSize)]; |
There was a problem hiding this comment.
dont use cfg as a variable name, use descriptive names
| autoScrollOffset = 30, | ||
| ...accessibilityProps | ||
| } = mergedProps; | ||
| const resolvedSize: ChipSize = size ?? (compact ? 'xs' : 's'); |
There was a problem hiding this comment.
i think we have already messed with the UI enough that we dont need to be so strict about keeping compact working exactly as it has before this release. Just update the code around compact everywhere in the changed compoentns so it just resolves to the cooresponding/appropriate size and the component can just render as that size. Hopefully this improves the complexity of some components who are maintaining logic just for legacy compact mode.
IMPORTANT - there is one exception. In places where the old compact used to force the label of an input to behave a certain way (TextInput for example) - compact should still continue to do this, but all other effects like the paddding can be defered to the resolved size
|
|
||
| const defaultSlideButtonSize: SlideButtonSize = 'l'; | ||
|
|
||
| export const resolveSlideButtonSize = ( |
There was a problem hiding this comment.
other components have just inlined this as render logic - we need to be consistent in implemenation strategies
Apply consistent size-config pattern across sized components and simplify compact handling per PR #796 review: - Derive size-config value types via Pick from the authoritative *BaseProps instead of hand-authored *SizeConfig type literals (Button, IconButton, SlideButton). - Rename terse `cfg` to `sizeConfig`; use a module-level default<Component>Size const with inline resolution everywhere; remove the resolveSlideButtonSize helper (resolve once, forward resolved size to sub-components). - Reword IconButton size/compact JSDoc to user-facing docs with @default l. - Simplify compact to resolve to its size equivalent and drop legacy-only branches, except keep TextInput's compact label-placement behavior. - Chip family: move size/compact onto the base Chip only (remove SelectChip overrides); restore mobile SelectChip's spread-last ordering; remove the byte-for-byte comment. - Preserve web TextInput data-compact so customer styles keyed on it keep working (matches master); update affected tests. - Remove low-value render-only tests. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
|
closing in favor of : #809 |
Summary
Rolls out the cross-platform t-shirt
sizeprop across CDS components, deprecating the legacycompactboolean in favor of the new sizing API. Each component's per-size styles are derived from its Figma component set and applied consistently across web and mobile. Backward compatibility is preserved throughout:compactkeeps working exactly as before (sizewins when both are set), no fixed heights are introduced (except Slide Button, which is inherently fixed-height by design sign-off), and no public exports are renamed or dropped.compactis deprecated with@deprecationExpectedRemoval v10.Delivered as 7 verified commits (unit tests + lint + typecheck + format green for each):
92928083b244bb0d2bf4454790be4d1eca30ea0327016d95eNotable decisions:
sizewas added to the shared baseChipandcompactdeprecated across the whole chip family;Input Chip,Select Chip, andTabbedChipsinherit from it (unionxs | s, defaults).cds-commongetMediaChipSpacingPropsis size-aware but backward compatible.sizeadded to bothDatePickerandDateInput(web + mobile); label placement is decoupled from size via TextInput's density logic (size="l"stacks inside labels vertically).mreusesl's handle icon size.Linear Issues
Deferred (not in this PR)
These hit hard stops during planning and need design/product input before implementation:
sizevariant; it is a layout-only wrapper that delegates sizing to child Buttons. Needs design to add asizevariant or a decision to forwardsizeto children.sizevariant in Figma and noIconButtonGroupcomponent exists in code (Code Connect maps it to plainButtonGroup). Needs the ticket reframed onto a real target component.Next steps
Intentionally deferred from this PR to keep scope focused on the size API + component/stories work:
apps/docs): Update component examples to document the newsizeprop and remove/replace deprecatedcompactexamples for each rolled-out component.__figma__templates): Refresh Code Connect mappings to include the newsizevariant property for each rolled-out component.Approach
This work was fully executed by agents orchestrated by a Claude Dynamic Workflow. The workflow is comprised of 3 phases: plan, review and implement, with the plan phase being fully parallel. To define a clear, repeatable workflow for the plan agents to follow, an agent skill was created (
cds-tshirt-sizes) and kept in this PR for illustrative purposes. We can remove this skill in a later PR since it is specific to only the work in this PR.Input prompt: