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

Restructure CurrencyDisplay to use Design System #19171

Merged
merged 13 commits into from
Jul 25, 2023

Conversation

darkwing
Copy link
Contributor

Explanation

The CurrencyDisplay component has legacy CSS and is unfriendly to the new design system. This is essentially a structural rewrite using the new design system components.

Screenshots/Screencaps

There should be no visible changes.

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • 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.

@darkwing darkwing requested a review from a team as a code owner May 16, 2023 14:54
@darkwing darkwing requested a review from legobeat May 16, 2023 14:54
@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.

@darkwing darkwing force-pushed the currency-display-rewrite branch from 2d0bbf8 to 16de22c Compare May 16, 2023 16:48
@darkwing darkwing force-pushed the currency-display-rewrite branch 3 times, most recently from d1a53b0 to 767d879 Compare May 26, 2023 18:51
vthomas13
vthomas13 previously approved these changes Jun 1, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [8026ab9]
Page Load Metrics (1786 ± 101 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint110194142199
domContentLoaded14822129175618991
load148222311786211101
domInteractive14822129175518991
Bundle size diffs
  • background: 0 bytes
  • ui: 986 bytes
  • common: 0 bytes

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 great! I like the inversion of control pattern you are implementing here! Less a-prop-calypses and more maintainable components for the win! Made some small suggestions for updating deprecated code and storybook. Checked the component on my local build

Screenshot 2023-06-09 at 11 42 36 AM

@darkwing darkwing force-pushed the currency-display-rewrite branch 2 times, most recently from b12c7e9 to 084d114 Compare June 13, 2023 18:00
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.

Code LGTM. Just need pesky e2e tests to pass

NidhiKJha
NidhiKJha previously approved these changes Jun 27, 2023
@NidhiKJha NidhiKJha added team-extension-client team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead labels Jul 6, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [987d548]
Page Load Metrics (1500 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint982781353818
domContentLoaded1388168915008440
load1388168915008440
domInteractive1388168915008440
Bundle size diffs
  • background: 0 bytes
  • ui: 890 bytes
  • common: 0 bytes

@metamaskbot
Copy link
Collaborator

Builds ready [48ded7b]
Page Load Metrics (1636 ± 65 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint971821412512
domContentLoaded14941981163513565
load14941981163613565
domInteractive14941981163513565
Bundle size diffs
  • background: 0 bytes
  • ui: 890 bytes
  • common: 0 bytes

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Merging #19171 (5d055d7) into develop (5d17f86) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop   #19171   +/-   ##
========================================
  Coverage    69.42%   69.43%           
========================================
  Files          990      990           
  Lines        37418    37423    +5     
  Branches     10039    10044    +5     
========================================
+ Hits         25976    25981    +5     
  Misses       11442    11442           
Impacted Files Coverage Δ
.../ui/currency-display/currency-display.component.js 100.00% <100.00%> (ø)

@darkwing darkwing dismissed stale reviews from NidhiKJha and georgewrmarshall via 07e6d59 July 17, 2023 21:20
@darkwing darkwing force-pushed the currency-display-rewrite branch from 48ded7b to 07e6d59 Compare July 17, 2023 21:20
@darkwing darkwing force-pushed the currency-display-rewrite branch from 07e6d59 to e6376e6 Compare July 17, 2023 21:24
@metamaskbot
Copy link
Collaborator

Builds ready [dc2497a]
Page Load Metrics (1615 ± 86 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1042731474019
domContentLoaded14262084161518086
load14272084161518086
domInteractive14262084161518086
Bundle size diffs
  • background: 0 bytes
  • ui: 980 bytes
  • common: 0 bytes

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

@garrettbear garrettbear left a comment

Choose a reason for hiding this comment

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

Aside from the comment about boxProps this looks good to me

@metamaskbot
Copy link
Collaborator

Builds ready [5d055d7]
Page Load Metrics (1509 ± 37 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint101159124126
domContentLoaded1403164515097837
load1403164515097837
domInteractive1403164515097837
Bundle size diffs
  • background: 0 bytes
  • ui: 945 bytes
  • common: 0 bytes

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.

I think the code looks good and I know this PR has been open for a while so this is non-blocking but could we update the story slightly more so it adds more value to the viewer? The default story has 7.3b but it's not reflected in the component. The second story seems to add more value. Can we consolidate them possibly?

Screenshot 2023-07-24 at 3 11 36 PM

@darkwing darkwing merged commit bab1670 into develop Jul 25, 2023
@darkwing darkwing deleted the currency-display-rewrite branch July 25, 2023 19:32
@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2023
@metamaskbot metamaskbot added the release-10.36.0 Issue or pull request that will be included in release 10.36.0 label Jul 25, 2023
@Gudahtt Gudahtt added release-11.1.0 Issue or pull request that will be included in release 11.1.0 and removed release-10.36.0 Issue or pull request that will be included in release 10.36.0 labels Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.1.0 Issue or pull request that will be included in release 11.1.0 team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants