feat(components): [DSYS-194] add description and errorMessage props to field components#1941
feat(components): [DSYS-194] add description and errorMessage props to field components#1941ShreyasGit51283 wants to merge 1 commit into
Conversation
…o field components Introduces a shared renderFieldHelpers() helper and adds ergonomic `description`/`errorMessage` props to TextField, NumberField, SearchField, Select, ComboBox, RadioGroup, CheckboxGroup, DateField, and TimeField. The props render the React Aria description slot and FieldError internally, matching the Figma library's flat `description` property and resolving the Code Connect drift. Additive and non-breaking: existing slotted children keep working. Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: a7e5e3a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a7e5e3a. Configure here.
| <Provider values={[[GroupContext, { isInvalid, isDisabled }]]}>{children}</Provider> | ||
| <Provider values={[[GroupContext, { isInvalid, isDisabled }]]}> | ||
| {renderFieldHelpers(children, { description, errorMessage })} | ||
| </Provider> |
There was a problem hiding this comment.
Context helper props ignored
Medium Severity
description and errorMessage are taken from props before useLPContextProps runs, so values supplied only through each field’s LaunchPad context never reach renderFieldHelpers. Those merged keys can still be spread onto the React Aria wrapper via {...props}.
Additional Locations (2)
Triggered by project rule: Bugbot Instructions — launchpad-ui
Reviewed by Cursor Bugbot for commit a7e5e3a. Configure here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| * https://react-spectrum.adobe.com/react-aria/CheckboxGroup.html | ||
| */ | ||
| const CheckboxGroup = ({ ref, ...props }: CheckboxGroupProps) => { | ||
| const CheckboxGroup = ({ ref, description, errorMessage, ...props }: CheckboxGroupProps) => { |
There was a problem hiding this comment.
🟡 Field helper props provided through context are silently ignored and leak to the DOM
The description and error message are extracted from the component's input ({ ref, description, errorMessage, ...props } at packages/components/src/CheckboxGroup.tsx:27) before context values are merged in, so any values supplied through the public context are never rendered as helper text and instead leak as unknown attributes.
Impact: Consumers using the exported context (e.g. CheckboxGroupContext) to set description or error message will see no visible helper text and may get React warnings about unrecognized DOM properties.
Destructuring order breaks the established context-merge pattern
The established pattern across all components in this codebase is to only destructure ref before calling useLPContextProps, because ref requires special handling via mergeRefs (packages/components/src/utils.tsx:88). All other props flow through context merging.
This PR destructures description and errorMessage before the context merge in all 9 affected components:
packages/components/src/CheckboxGroup.tsx:27packages/components/src/RadioGroup.tsx:27packages/components/src/TextField.tsx:29packages/components/src/NumberField.tsx:27packages/components/src/SearchField.tsx:27packages/components/src/DateField.tsx:58-63packages/components/src/DateField.tsx:119-124(TimeField)packages/components/src/Select.tsx:42packages/components/src/ComboBox.tsx:36-41
The context types include FieldHelperProps because they're typed as ContextValue<XxxProps, HTMLDivElement> and XxxProps now extends FieldHelperProps. So TypeScript allows providing these via context, but the runtime ignores them.
The fix is to destructure description and errorMessage AFTER useLPContextProps, e.g.:
const CheckboxGroup = ({ ref, ...props }: CheckboxGroupProps) => {
[props, ref] = useLPContextProps(props, ref, CheckboxGroupContext);
const { description, errorMessage, ...restProps } = props;
return (
<AriaCheckboxGroup {...restProps} ref={ref} ...>
{composeRenderProps(restProps.children, (children) =>
renderFieldHelpers(children, { description, errorMessage }),
)}
</AriaCheckboxGroup>
);
};Prompt for agents
In all 9 affected field components (CheckboxGroup, RadioGroup, TextField, NumberField, SearchField, DateField, TimeField, Select, ComboBox), the description and errorMessage props are destructured from the component arguments BEFORE useLPContextProps merges context-provided values. This breaks the established pattern where only ref is destructured before context merging.
The fix for each component is to move the destructuring of description and errorMessage to AFTER the useLPContextProps call, and then pass the remaining props (without description/errorMessage) to the underlying RAC component. For example in CheckboxGroup.tsx:
1. Change the function signature to only destructure ref: ({ ref, ...props })
2. After useLPContextProps, destructure: const { description, errorMessage, ...restProps } = props;
3. Pass restProps (not props) to AriaCheckboxGroup and restProps.children to composeRenderProps
4. Continue passing description and errorMessage to renderFieldHelpers
Apply the same pattern to all 9 components: CheckboxGroup.tsx, RadioGroup.tsx, TextField.tsx, NumberField.tsx, SearchField.tsx, DateField.tsx (both DateField and TimeField), Select.tsx, and ComboBox.tsx.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const renderFieldHelpers = ( | ||
| children: ReactNode, | ||
| { description, errorMessage }: FieldHelperProps, | ||
| ) => ( | ||
| <> | ||
| {children} | ||
| {description != null && <Text slot="description">{description}</Text>} | ||
| {errorMessage != null && <FieldError>{errorMessage}</FieldError>} | ||
| </> | ||
| ); |
There was a problem hiding this comment.
@apucacao @zakk-verrilli-ld
we've always approach launchpad component in composable architecture, and this approach seems to go against that.
do we want to expose description/error as props? or just direct develops to use the slot to render error/descriptions?
There was a problem hiding this comment.
Seems simpler to me do use the slot API, but I don't have much context around this change. Is this just about ergonomics? Agents and humans seem to deal with slots fine, and — assuming it is mostly about ergonomics — I'd argue adding another mechanism of providing values for these slots could add confusion: 2 ways of doing the exact same thing?
There was a problem hiding this comment.
Agreed slots are the way. Both description and errorMessage are (in some cases the exclusive) slots for field components already.
Is the issue here that Figma Code Connect is not wired up to generate the subcomponents? Slack Thread
|
@zakk-verrilli-ld reccomends we should explore fixing the Code connect generator. Closing this PR to explore that route |
4 participants
Summary
Resolves the
descriptionprop drift between the LaunchPad React components and the Figma library (DSYS-194).The Figma library exposes a flat
descriptionproperty on field components, but in code helper text was only expressible as a slotted child (<Text slot="description">). This mismatch broke Code Connect mappings.This PR adds ergonomic
descriptionanderrorMessageprops to the field wrappers, implemented through a single shared helper so the slot-rendering logic lives in exactly one place.renderFieldHelpers()+FieldHelperPropsinsrc/fieldHelpers.tsx— the single source of truth that renders the RAC<Text slot="description">and<FieldError>slots.TextField,NumberField,SearchField,Select,ComboBox,RadioGroup,CheckboxGroup,DateField,TimeField.Checkbox,Switch,Radio,Label) intentionally not changed — per RAC/React Spectrum convention, helper text belongs at the group level..figma.tsx) mappings updated to map Figma'sdescriptionto the new prop.TextFieldstory updated (prop + back-compat slot story) with aplaytest asserting render +aria-describedbywiring.minorchangeset (additive, non-breaking — existing slotted children keep working).Review focus
Please review
TextFieldfirst — it is the reference implementation. Scrutinize:src/fieldHelpers.tsx(the shared helper — the only real logic)src/TextField.tsxfigma/TextField.figma.tsxThe other 8 components apply the identical mechanical change: destructure
description, errorMessageout of props and route children throughrenderFieldHelpers. Once theTextFieldapproach is approved, the rest are replicas.Test plan
pnpm --filter @launchpad-ui/components test— run theTextFieldplaytestTextFieldExamplerenders the description andDescriptionAsSlotstill worksfigma connect publishvalidation — confirm the FigmaDescriptionproperty/layer names in the updated.figma.tsxfiles match the actual Figma components (several were added following the existing'Description?'/'Description'convention and need verification against the live file)Made with Cursor