-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrates avatar base to TypeScript #18494
Migrates avatar base to TypeScript #18494
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. |
…nd/metamask-extension into migrate_avatar_base_ts Merge .
ui/components/component-library/avatar-base/avatar-base.types.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if Text being in TS is a dependency of this PR?
ui/components/component-library/avatar-base/avatar-base.types.ts
Outdated
Show resolved
Hide resolved
ui/components/component-library/avatar-base/avatar-base.types.ts
Outdated
Show resolved
Hide resolved
@georgewrmarshall I am getting the error |
@georgewrmarshall what is the update on my questions? |
Hi @spiritanand, I'm not sure what your issue is from a quick glance. I'll try do some digging this week. It also looks like you have a failing unit test. |
Are you referring to this? |
Thanks for the catch @legobeat, that was something I missed as well. The linting errors that I am getting are, I have fixed the issue you have pointed out above, though. Added |
It is working perfectly. Thank you, @legobeat and @georgewrmarshall You guys have been tremendously patient and helpful :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Left 2 small suggestions
ui/components/component-library/avatar-base/avatar-base.stories.tsx
Outdated
Show resolved
Hide resolved
…ildren attribute from AvatarBase storybook file
Done :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 💯 Thank you for you contribution @spiritanand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Explanation
Currently, the AvatarBase component is built in JavaScript. This PR deprecates the JS component and creates a new TypeScript version.
Migrated all
.js
to.tsx
or.ts
filesScreenshots/Screencaps
Before
Before.AvatarBase.mp4
After
No changes to the functionality of the component, stories, and controls all work. Updated docs.
screen-recording-2023-04-07-at-123922-pm_0cUhUGas.mp4
Manual Testing Steps
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 Board
label.In this case, a QA Engineer approval will be required.