Skip to content
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

Migrate AvatarIcon #19107

Merged
merged 4 commits into from
Jul 26, 2023
Merged

Conversation

thebinij
Copy link
Contributor

@thebinij thebinij commented May 12, 2023

Explanation

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.

@thebinij thebinij requested a review from a team as a code owner May 12, 2023 08:58
@thebinij thebinij requested a review from digiwand May 12, 2023 08:58
@github-actions
Copy link
Contributor

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 thebinij mentioned this pull request May 12, 2023
8 tasks
@thebinij
Copy link
Contributor Author

Hi @georgewrmarshall, Please review this PR. Thanks!

@codecov
Copy link

codecov bot commented May 12, 2023

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 georgewrmarshall added the team-design-system All issues relating to design system in Extension label May 14, 2023
Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Left a suggestion

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

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one suggestion

@thebinij thebinij requested review from a team, kumavis and brad-decker as code owners May 18, 2023 11:46
@thebinij thebinij force-pushed the migrate/18883-AvatarIcon branch from 6a93203 to f8fd84a Compare May 18, 2023 12:44
@thebinij thebinij mentioned this pull request May 18, 2023
6 tasks
Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a suggestion about mapping the size prop

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

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 thebinij force-pushed the migrate/18883-AvatarIcon branch from b38b047 to e852873 Compare June 13, 2023 17:11
@thebinij thebinij force-pushed the migrate/18883-AvatarIcon branch from e852873 to b8e8b66 Compare June 25, 2023 05:59
@thebinij
Copy link
Contributor Author

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

@georgewrmarshall
Copy link
Contributor

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 garrettbear force-pushed the migrate/18883-AvatarIcon branch from b8e8b66 to 89696c3 Compare July 26, 2023 16:06
@garrettbear
Copy link
Contributor

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
Copy link
Contributor

Before

Screenshot 2023-07-26 at 10 25 54 AM Screenshot 2023-07-26 at 10 25 29 AM

After

Screenshot 2023-07-26 at 10 25 44 AM Screenshot 2023-07-26 at 10 25 38 AM

garrettbear
garrettbear previously approved these changes Jul 26, 2023
Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
} as Meta<typeof AvatarIcon>;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After

after.mov

@garrettbear garrettbear merged commit 6e5ab2a into MetaMask:develop Jul 26, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
external-contributor team-design-system All issues relating to design system in Extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate components to TS: AvatarIcon
3 participants