Migrate AvatarIcon #19107

thebinij · 2023-05-12T08:58:53Z

Explanation

Closes Migrate components to TS: AvatarIcon #18883

Screenshots/Screencaps

Before

output-before.mp4

After

output-after.mp4

Manual Testing Steps

No functional changes

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

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 be required.

github-actions · 2023-05-12T08:59:06Z

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.

thebinij · 2023-05-12T09:40:18Z

Hi @georgewrmarshall, Please review this PR. Thanks!

codecov · 2023-05-12T10:05:02Z

Codecov Report

Merging #19107 (df3b502) into develop (4b2c777) will decrease coverage by 0.00%.
Report is 2 commits behind head on develop.
The diff coverage is 100.00%.

❗ Current head df3b502 differs from pull request most recent head ce01147. Consider uploading reports for the commit ce01147 to get more accurate results

@@             Coverage Diff             @@
##           develop   #19107      +/-   ##
===========================================
- Coverage    68.67%   68.67%   -0.00%     
===========================================
  Files          986      986              
  Lines        37862    37863       +1     
  Branches     10135    10136       +1     
===========================================
- Hits         26000    25999       -1     
- Misses       11862    11864       +2

Files Changed	Coverage Δ
...ents/component-library/avatar-icon/avatar-icon.tsx	`100.00% <100.00%> (ø)`
...component-library/avatar-icon/avatar-icon.types.ts	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

georgewrmarshall

Looking good. Left a suggestion

ui/components/component-library/avatar-icon/avatar-icon.types.ts

ui/components/component-library/avatar-icon/avatar-icon.tsx

ui/components/component-library/avatar-icon/avatar-icon.types.ts

georgewrmarshall

Left one suggestion

georgewrmarshall

Left a suggestion about mapping the size prop

ui/components/component-library/avatar-icon/avatar-icon.tsx

georgewrmarshall · 2023-05-31T22:20:21Z

Hey @thebinij, Thanks for your time and effort in creating this pull request. As you're probably aware we have identified an issue related to the typing of the Box component, specifically with the polymorphic as prop. To address this issue, we have an open ticket at #19239 and a draft pull request at #19363 that needs to be merged before we can proceed with your PR. Thanks for your patience

thebinij · 2023-06-25T06:00:01Z

Hi @georgewrmarshall, Is there anything I can update in the PR before so you approve to merge it?

georgewrmarshall · 2023-06-26T21:10:03Z

Blocked by #19572

# This is the 1st commit message: Migrate AvatarIcon # This is the commit message MetaMask#2: fixing lint

fixing lint fixing size issue IconProps extends BaseProps instead of BoxProps Omit children from IconProps fixing lint issues mapping avatarIconSize to Iconsize replace deprecated

garrettbear · 2023-07-26T17:24:53Z

AvatarIcon looks good. When reviewing AvatarIcon in production, noticed some improvements that could be made on both our end and help instill best practices for other devs. Created a separate ticket for this issue since it would make this PR more complicated than it needs to be. @georgewrmarshall

garrettbear · 2023-07-26T17:27:21Z

Before

After

georgewrmarshall

LGTM! Made a tiny change to make the storybook controls to work as intended for the iconName prop. Thanks for your contribution @thebinij and support @garrettbear

georgewrmarshall · 2023-07-26T20:51:04Z

ui/components/component-library/avatar-icon/avatar-icon.stories.tsx

  },
 } as Meta<typeof AvatarIcon>;

 const Template: StoryFn<typeof AvatarIcon> = (args) => {
-  return <AvatarIcon {...args} iconName={IconName.SwapHorizontal} />;
+  return <AvatarIcon {...args} />;
 };


After

after.mov

thebinij requested a review from a team as a code owner May 12, 2023 08:58

thebinij requested a review from digiwand May 12, 2023 08:58

thebinij mentioned this pull request May 12, 2023

Migrate-AvatarIcon #19023

Closed

8 tasks

georgewrmarshall added the team-design-system All issues relating to design system in Extension label May 14, 2023

georgewrmarshall reviewed May 14, 2023

View reviewed changes

ui/components/component-library/avatar-icon/avatar-icon.types.ts Outdated Show resolved Hide resolved

thebinij commented May 14, 2023

View reviewed changes

ui/components/component-library/avatar-icon/avatar-icon.tsx Outdated Show resolved Hide resolved

thebinij commented May 14, 2023

View reviewed changes

ui/components/component-library/avatar-icon/avatar-icon.types.ts Show resolved Hide resolved

georgewrmarshall requested review from garrettbear and removed request for digiwand May 15, 2023 22:39

georgewrmarshall reviewed May 18, 2023

View reviewed changes

ui/components/component-library/avatar-icon/avatar-icon.types.ts Outdated Show resolved Hide resolved

georgewrmarshall reviewed May 18, 2023

View reviewed changes

thebinij requested review from a team, kumavis and brad-decker as code owners May 18, 2023 11:46

thebinij force-pushed the migrate/18883-AvatarIcon branch from 6a93203 to f8fd84a Compare May 18, 2023 12:44

thebinij mentioned this pull request May 18, 2023

Fix/18884 migrate avatar network #19079

Merged

6 tasks

georgewrmarshall reviewed May 24, 2023

View reviewed changes

ui/components/component-library/avatar-icon/avatar-icon.tsx Outdated Show resolved Hide resolved

thebinij force-pushed the migrate/18883-AvatarIcon branch 2 times, most recently from 1d73752 to b38b047 Compare May 29, 2023 06:54

thebinij force-pushed the migrate/18883-AvatarIcon branch from b38b047 to e852873 Compare June 13, 2023 17:11

thebinij force-pushed the migrate/18883-AvatarIcon branch from e852873 to b8e8b66 Compare June 25, 2023 05:59

# This is a combination of 2 commits.

265a209

# This is the 1st commit message: Migrate AvatarIcon # This is the commit message MetaMask#2: fixing lint

Migrate AvatarIcon

89696c3

fixing lint fixing size issue IconProps extends BaseProps instead of BoxProps Omit children from IconProps fixing lint issues mapping avatarIconSize to Iconsize replace deprecated

garrettbear force-pushed the migrate/18883-AvatarIcon branch from b8e8b66 to 89696c3 Compare July 26, 2023 16:06

AvatarIcon cleanup

df3b502

garrettbear requested a review from georgewrmarshall July 26, 2023 17:14

garrettbear previously approved these changes Jul 26, 2023

View reviewed changes

Small update to allow storybook iconName control to work

ce01147

georgewrmarshall dismissed garrettbear’s stale review via ce01147 July 26, 2023 20:47

georgewrmarshall approved these changes Jul 26, 2023

View reviewed changes

georgewrmarshall reviewed Jul 26, 2023

View reviewed changes

garrettbear approved these changes Jul 26, 2023

View reviewed changes

garrettbear merged commit 6e5ab2a into MetaMask:develop Jul 26, 2023
9 checks passed

github-actions bot locked and limited conversation to collaborators Jul 26, 2023

georgewrmarshall added the external-contributor label Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate AvatarIcon #19107

Migrate AvatarIcon #19107

thebinij commented May 12, 2023 •

edited by georgewrmarshall

Loading

github-actions bot commented May 12, 2023

thebinij commented May 12, 2023

codecov bot commented May 12, 2023 •

edited

Loading

georgewrmarshall left a comment

georgewrmarshall left a comment

georgewrmarshall left a comment

georgewrmarshall commented May 31, 2023

thebinij commented Jun 25, 2023

georgewrmarshall commented Jun 26, 2023

garrettbear commented Jul 26, 2023

garrettbear commented Jul 26, 2023

georgewrmarshall left a comment

georgewrmarshall Jul 26, 2023

Migrate AvatarIcon #19107

Migrate AvatarIcon #19107

Conversation

thebinij commented May 12, 2023 • edited by georgewrmarshall Loading

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented May 12, 2023

thebinij commented May 12, 2023

codecov bot commented May 12, 2023 • edited Loading

Codecov Report

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall commented May 31, 2023

thebinij commented Jun 25, 2023

georgewrmarshall commented Jun 26, 2023

garrettbear commented Jul 26, 2023

garrettbear commented Jul 26, 2023

Before

After

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall Jul 26, 2023

Choose a reason for hiding this comment

After

thebinij commented May 12, 2023 •

edited by georgewrmarshall

Loading

codecov bot commented May 12, 2023 •

edited

Loading