feat: remove dependency on o-utils from o3-form, require inputId#2365
feat: remove dependency on o-utils from o3-form, require inputId#2365
Conversation
| feedback={feedback} | ||
| description={description} | ||
| inputId={id} | ||
| inputId={inputId} |
There was a problem hiding this comment.
I find this prop to be misleading. In most components, the prop is used to assign an ID to the HTMLElement. However in this case it is used as a reference for htmlFor
There was a problem hiding this comment.
What I meant by good change is the value for the input is clearer even though it says inputId in there already 😂
There was a problem hiding this comment.
Pull request overview
This PR updates o3-form to remove its reliance on @financial-times/o-utils for ID generation by making inputId required and wiring all rendered id/htmlFor references directly to that value.
Changes:
- Make
inputIda required prop acrosso3-forminput/fieldset types. - Remove
uidBuilderusage in form components and derive related element IDs frominputId. - Drop
@financial-times/o-utilsfromo3-formpeer dependencies (and update the lockfile accordingly).
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package-lock.json | Removes o-utils peer dep entry for o3-form and bumps o-teaser version. |
| components/o3-form/package.json | Removes @financial-times/o-utils from peerDependencies. |
| components/o3-form/src/types/index.ts | Makes inputId required on base/select/fieldset-related props. |
| components/o3-form/src/tsx/fieldComponents/FormField.tsx | Removes generated IDs; ties label/description IDs to inputId. |
| components/o3-form/src/tsx/TextInput.tsx | Removes generated fallback ID; uses inputId directly. |
| components/o3-form/src/tsx/SelectInput.tsx | Removes generated fallback ID; uses inputId directly. |
| components/o3-form/src/tsx/RadioButton.tsx | Removes generated fallback ID for radio items. |
| components/o3-form/src/tsx/PasswordInput.tsx | Removes generated fallback ID; uses inputId directly. |
| components/o3-form/src/tsx/FileInput.tsx | Removes generated fallback ID; uses inputId directly. |
| components/o3-form/src/tsx/DateInputPicker.tsx | Removes generated fallback ID; uses inputId directly. |
| components/o3-form/src/tsx/DateInput.tsx | Removes generated fallback ID; uses inputId directly. |
| components/o3-form/src/tsx/CheckBox.tsx | Removes generated fallback ID for checkbox items. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Base Input Props | ||
| export type BaseInputProps = { | ||
| inputId?: string; | ||
| inputId: string; | ||
| label?: string; |
There was a problem hiding this comment.
inputId is now required on BaseInputProps, SelectInputProps, and FormFieldsetProps, but existing in-repo usages (e.g. Storybook stories and README TSX snippets) still omit inputId. This will cause TypeScript build/story/test failures unless those call sites are updated (or the components keep a fallback ID generation).
inputId is misleading on LabaledFormField as it does not provide an ID to that element. Instead, it is used as a ref in htmlFor.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
components/o3-form/src/types/index.ts:9
BaseInputPropsnow requireslabel, which forces components likeCheckBoxItem/RadioButtonItem(which usecheckboxLabel/radioButtonLabel) to supply an unusedlabelprop and breaks existing call sites. Consider splitting the shared props (e.g.,inputId-only base) from “labeled field” props, or keeplabeloptional on the base type and require it only for components that actually render it. Also note that makinginputIdrequired in TS doesn’t enforce it for JS consumers—consider a runtime invariant/throw when it’s missing/empty to avoid renderingid={undefined}.
export type BaseInputProps = {
inputId: string;
label: string;
description?: string;
optional?: boolean;
error?: boolean;
attributes?: JSX.IntrinsicElements['input'];
};
components/o3-form/src/tsx/fieldComponents/FormField.tsx:85
FormFieldsetalways setsaria-describedbytocheckbox_${inputId}, even whendescriptionis not provided and the element with that id is never rendered. This produces anaria-describedbyreference to a non-existent element; setaria-describedbyonly whendescriptionis present (or render the described element unconditionally).
const descriptionId = `checkbox_${inputId}`;
const labelClasses = ['o3-form-field__legend'];
if (optional) {
labelClasses.push('o3-form-field--optional');
}
return (
<fieldset className="o3-form-field" aria-describedby={`${descriptionId}`}>
<legend className={labelClasses.join(' ')}>
{label}{' '}
{description && (
<span className="o3-form-input__description" id={descriptionId}>
{description}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@joelcarr and I looked at this in the Murder of Devs meeting today and addressed quite a few concerns. |
No description provided.