chore: removing deprecated secondary colors from codebase by georgewrmarshall · Pull Request #24971 · MetaMask/metamask-extension

georgewrmarshall · 2024-06-01T00:16:12Z

Description

This pull request removes the deprecated secondary colors in preparation for upgrading to design token v4 and priming the brand evolution. The updates all Spinner component instances to use the default icon-muted color.

Dependency on

Related issues

Fixes: #24964

Manual testing steps

Go to the storybook build of this PR
Go to all stories that use the Spinner component: Spinner, AppLoadingSpinner, EthOverview, QRCodeModal, LoadingScreen (some stories not included in PR as they didn't add much value)
See color updates are working as expected
Pull this branch search Spinner
Ensure no instances have an override color
Search for color-secondary to ensure no instances of secondary colors remain

Screenshots/Recordings

All before/after screenshots and screencasts of Spinner instnaces have been moved to comments for easier review.

Below screenshot shows all Spinner instances have removed color prop ensuring colors have been consolidated to use the default icon-muted color

Pre-merge author checklist

I’ve followed MetaMask Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-06-01T00:16:29Z

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.

georgewrmarshall · 2024-06-01T00:30:06Z

.storybook/3.COLORS.stories.mdx

-var(--color-secondary-default) [DEPRECATED]
-var(--color-secondary-alternative) [DEPRECATED]
-var(--color-secondary-muted) [DEPRECATED]
-var(--color-secondary-inverse) [DEPRECATED]
-var(--color-secondary-disabled) [DEPRECATED]
-


Updating color docs to remove references to the removed secondary CSS variables.

Before

color.docs.secondary720.mov

After

after.color.docs720.mov

ui/components/app/wallet-overview/eth-overview.js

ui/components/app/app-loading-spinner/app-loading-spinner.stories.js

ui/components/app/wallet-overview/eth-overview.stories.js

ui/components/app/app-loading-spinner/app-loading-spinner.js

ui/components/ui/spinner/spinner.stories.js

ui/components/app/modals/qr-scanner/qr-scanner.component.js

ui/components/institutional/qr-code-modal/qr-code-modal.tsx

georgewrmarshall · 2024-06-03T20:43:07Z

ui/components/app/qr-hardware-popover/enhanced-reader.js

      />
-      {canplay ? null : <Spinner color="var(--color-warning-default)" />}
+      {canplay ? null : <Spinner color="var(--color-icon-muted)" />}
    </div>


Story created to capture this recording but not included in the PR as it doesn't add much value, only showing the spinner component. The recording shows that the spinner uses the default icon-muted color.

After

after720.mov

ui/components/institutional/qr-code-modal/qr-code-modal.stories.tsx

ui/components/institutional/qr-code-modal/qr-code-modal.tsx

ui/components/ui/loading-screen/loading-screen.component.js

ui/components/ui/loading-screen/loading-screen.stories.js

ui/components/ui/spinner/spinner.component.js

codecov · 2024-06-03T21:34:16Z

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 65.64%. Comparing base (ed5ec2d) to head (5c7aa19).
Report is 7 commits behind head on develop.

Files	Patch %	Lines
...ents/app/modals/qr-scanner/qr-scanner.component.js	0.00%	1 Missing ⚠️
...ponents/app/qr-hardware-popover/enhanced-reader.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #24971   +/-   ##
========================================
  Coverage    65.64%   65.64%           
========================================
  Files         1362     1362           
  Lines        54189    54187    -2     
  Branches     14112    14110    -2     
========================================
- Hits         35572    35571    -1     
+ Misses       18617    18616    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

brianacnguyen

This PR can be broken down to at least 2-3 PRs
1/ qr-code-modal update
2/ storybook cleanup
3/ removing secondary colors

georgewrmarshall · 2024-06-05T23:03:24Z

This PR can be broken down to at least 2-3 PRs 1/ qr-code-modal update 2/ storybook cleanup 3/ removing secondary colors

Good suggestion I've broken this PR up see below

chore: adding storybook stories to review colors #25083 - chore: adding storybook stories
chore: consolidating spinner color and refactoring institutional qr code modal #25011 - chore: refactoring institutional qr code modal
chore: removing deprecated secondary colors from codebase #24971 - removing secondary colors (this one)

metamaskbot · 2024-06-06T16:39:42Z

Builds ready [8c9c1ba]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (201 ± 207 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	75	118	92	11	5
		domContentLoaded	9	31	13	7	3
		load	44	1648	201	430	207
		domInteractive	9	31	13	7	3

## **Description** Breaking out parts of #24971 for easier review. This PR replaces deprecated colors in favor of the most updated design system colors. I also noticed that this particular component could benefit from some layout and overflow updates. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25011?quickstart=1) ## **Related issues** Part of: #24964 ## **Manual testing steps** 1. Go to the storybook build of this page 2. Search for `Spinner` and `QRCodeModal` 3. Check colors have been updated properly in light and dark mode ## **Screenshots/Recordings** ### **Before** https://github.com/MetaMask/metamask-extension/assets/8112138/d745a119-f42d-429e-9200-d69b5a1776d1 ### **After** https://github.com/MetaMask/metamask-extension/assets/8112138/282e4460-798a-4841-bad5-242373f7f89b ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

ui/components/app/wallet-overview/coin-overview.tsx

metamaskbot · 2024-06-07T21:00:39Z

Builds ready [d09edff]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (185 ± 196 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	67	92	82	7	4
		domContentLoaded	8	19	11	2	1
		load	39	1513	185	409	196
		domInteractive	8	19	11	2	1

Bundle size diffs

background: 0 Bytes (0.00%)
ui: -181 Bytes (-0.00%)
common: 0 Bytes (0.00%)

georgewrmarshall · 2024-06-07T22:18:46Z

ui/components/app/modals/qr-scanner/qr-scanner.component.js

After

Removing the secondary color override so it uses the default color, which is set to icon-muted. The story was created for this recording and is not included in the PR due to an infinite loop when trying to access the browser's camera in this component. Also, I don't think dark mode works if the page is refreshed, but it uses the default spinner color that has been tested in dark mode in other recordings.

after720.mov

georgewrmarshall · 2024-06-07T22:18:48Z

ui/components/app/app-loading-spinner/app-loading-spinner.js

Removing the secondary color override so it uses the default color, which is set to icon-muted.

After

after720.mov

georgewrmarshall · 2024-06-07T22:22:11Z

ui/components/app/wallet-overview/coin-overview.tsx

You should be able to view this story in the storybook build of this PR. The recording shows the spinner using the default icon-muted color.

After

after720.mov

georgewrmarshall · 2024-06-07T22:23:08Z

ui/components/ui/loading-screen/loading-screen.component.js

You should be able to view this story in the storybook build of this PR. The recording shows the spinner using the default icon-muted color.

After

after720.mov

georgewrmarshall · 2024-06-07T22:27:48Z

ui/pages/confirmations/confirm-transaction/__snapshots__/confirm-transaction.test.js.snap

Updates snapshot

metamaskbot · 2024-06-10T05:14:16Z

Builds ready [5c7aa19]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (134 ± 171 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	67	105	85	10	5
		domContentLoaded	8	16	11	2	1
		load	41	1690	134	357	171
		domInteractive	8	16	11	2	1

Bundle size diffs

background: 0 Bytes (0.00%)
ui: -181 Bytes (-0.00%)
common: 0 Bytes (0.00%)

georgewrmarshall added the team-design-system All issues relating to design system in Extension label Jun 1, 2024

georgewrmarshall self-assigned this Jun 1, 2024

georgewrmarshall commented Jun 1, 2024

View reviewed changes

ui/components/app/wallet-overview/eth-overview.js Outdated Show resolved Hide resolved

georgewrmarshall commented Jun 1, 2024

View reviewed changes

ui/components/app/app-loading-spinner/app-loading-spinner.stories.js Show resolved Hide resolved

georgewrmarshall commented Jun 1, 2024

View reviewed changes

ui/components/app/wallet-overview/eth-overview.stories.js Outdated Show resolved Hide resolved

georgewrmarshall commented Jun 1, 2024

View reviewed changes

ui/components/app/app-loading-spinner/app-loading-spinner.js Outdated Show resolved Hide resolved

georgewrmarshall force-pushed the remove-secondary-colors branch from 99c8602 to 7d2a789 Compare June 3, 2024 17:10