Migrate FormTextField to TS#22302
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
| // Validation function for 'id' prop | ||
| const validateIdProp = () => { | ||
| if (label && !id) { | ||
| throw new Error( | ||
| `If a label prop exists, you must provide an 'id' prop for the label's htmlFor attribute for accessibility. Warning coming from FormTextField component.`, | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| // Invoke the validation function | ||
| React.useEffect(() => { | ||
| validateIdProp(); | ||
| }, [label, id]); |
There was a problem hiding this comment.
@georgewrmarshall this is another one of those proptypes throwing a curve ball at me
The original JS version:
/**
* The id of the FormTextField
* Required if label prop exists to ensure accessibility
*
* @param {object} props - The props passed to the component.
* @param {string} propName - The prop name in this case 'id'.
* @param {string} componentName - The name of the component.
*/
id: (props, propName, componentName) => {
if (props.label && !props[propName]) {
return new Error(
`If a label prop exists you must provide an ${propName} prop for the label's htmlFor attribute for accessibility. Warning coming from ${componentName} ui/components/component-library/form-text-field/form-text-field.js`,
);
}
return null;
},There was a problem hiding this comment.
Can we check if label is always used in the instances for this component and if so we could just make id a required prop? Otherwise I'll have to have a think about it when I get back tomorrow
georgewrmarshall
left a comment
There was a problem hiding this comment.
Looking good! Reviewed on my phone so will do a full review tomorrow
| /** | ||
| * Props for the TextField component within the FormTextField | ||
| */ | ||
| textFieldProps?: TextFieldProps<'input'>; |
There was a problem hiding this comment.
I believe the wrapper that receives the spread props is still a div
| textFieldProps?: TextFieldProps<'input'>; | |
| textFieldProps?: TextFieldProps<'div'>; |
There was a problem hiding this comment.
Then does TextField need to be changed as well?
There was a problem hiding this comment.
We are treating the prop types as an input even though the wrapper is a div
There was a problem hiding this comment.
Indeed, it should. Here, the input in ElementType refers to all props spread to the wrapper element, which is a div in this case. Therefore, we should be accepting valid div attributes. The attributes specific to the input element are defined in inputProps as seen here: https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/component-library/text-field/text-field.types.ts#L61
ui/components/component-library/form-text-field/form-text-field.tsx
Outdated
Show resolved
Hide resolved
| // Validation function for 'id' prop | ||
| const validateIdProp = () => { | ||
| if (label && !id) { | ||
| throw new Error( | ||
| `If a label prop exists, you must provide an 'id' prop for the label's htmlFor attribute for accessibility. Warning coming from FormTextField component.`, | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| // Invoke the validation function | ||
| React.useEffect(() => { | ||
| validateIdProp(); | ||
| }, [label, id]); |
There was a problem hiding this comment.
Can we check if label is always used in the instances for this component and if so we could just make id a required prop? Otherwise I'll have to have a think about it when I get back tomorrow
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #22302 +/- ##
===========================================
- Coverage 67.96% 67.91% -0.05%
===========================================
Files 1067 1072 +5
Lines 41276 41393 +117
Branches 11084 11124 +40
===========================================
+ Hits 28051 28111 +60
- Misses 13225 13282 +57 ☔ View full report in Codecov by Sentry. |
Builds ready [15e4996]
Page Load Metrics (1150 ± 159 ms)
Bundle size diffs
|
15e4996 to
d104594
Compare
georgewrmarshall
left a comment
There was a problem hiding this comment.
Looks good! Left one suggestion in the types regarding the wrapping html element otherwise LGTM!
- Checked stories, docs and controls ✅
- Checked all instances of
<FormTextField>were pointing to deprecated version ✅
ui/components/component-library/form-text-field/form-text-field.types.ts
Outdated
Show resolved
Hide resolved
…d.types.ts Co-authored-by: George Marshall <george.marshall@consensys.net>
Builds ready [37ba33f]
Page Load Metrics (1367 ± 127 ms)
Bundle size diffs
|
Description
Move
FormTextFieldjs version into deprecated folder with deprecation noticeUpdate import of any production uses for
FormTextFieldto the js versionUpdate/create TS version of FormTextField
Related issues
Fixes: #19120
Manual testing steps
Screenshots/Recordings
Before
After
Screen.Recording.2023-12-19.at.12.46.59.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist