Migrate Label component to TS #19146
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. |
|
I have read the CLA Document and I hereby sign the CLA |
georgewrmarshall
added
the
team-design-system
There was a problem hiding this comment.
Hey @gabrielmj23, thanks for your contribution this is looking great. There are some conventions in other TS component files that would be great if we could follow for the Label component. You could checkout the BadgeWrapper component files. In particular the label.types.ts file
| export const Label: React.FC<{ | ||
| htmlFor?: string; | ||
| className?: string; | ||
| children: string | React.ReactNode; | ||
| }> = ({ htmlFor, className, children, ...props }) => ( |
There was a problem hiding this comment.
Would it be possible to move these to a .types.ts file and export the LabelProps that will match our other component conventions. Could we also forwardRef to the Text component. I think all of our components should forward refs
There was a problem hiding this comment.
Thanks for the suggestions, I made two commits with the changes
| /** | ||
| * The id of the input associated with the label (in case Text is used as a Label) | ||
| */ | ||
| htmlFor?: string; |
There was a problem hiding this comment.
Going to work on a fixing the Box types to allow for polymorphic props so we can pass html attributes down to the html element without having to adding types to the original types. We can merge this and I'll work on it next.
|
Oh look like you may have to rebase |
|
Tried to rebase there, hope it works, let me know |
georgewrmarshall
force-pushed
the
19123-migrate-label-to-ts
branch
from
4a5cde6
to
36337a7
Compare
|
Hey @gabrielmj23, Thanks for your time and effort in creating this pull request. We have identified an issue related to the typing of the |
|
Blocked by #19949 |
garrettbear
force-pushed
the
19123-migrate-label-to-ts
branch
from
36337a7
to
14c63ae
Compare
Codecov Report
@@ Coverage Diff @@
## develop #19146 +/- ##
===========================================
- Coverage 69.38% 69.37% -0.00%
===========================================
Files 987 987
Lines 37267 37268 +1
Branches 10008 10009 +1
===========================================
- Hits 25854 25853 -1
- Misses 11413 11415 +2
|
Explanation
Fixes #19123
Update
Labelcomponent in https://github.com/MetaMask/metamask-extension/tree/e5fdb72acc03ce1932b7ad51c788f141a884acd9/ui/components/component-library/label to Typescript.I've also gone ahead and changed the
DISPLAYandFLEX_DIRECTIONconstant helper objects in the design system into enums. I'm seeing there's an open issue for this, but I've kept the same expected enum names.Also added an optional
htmlForfield to theTextcomponent, given thatLabeluses it to render and Typescript doesn't like passing thehtmlForfield when it's not part ofText. Shouldn't break anything as it's optional, and specified in documentation.Screenshots/Screencaps
No visual regression was observed when checking the component on Storybook.
Before
After
Manual Testing Steps
Running
yarn jest ui/components/component-library/label, passes all tests for this component.Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.