Updating Text component to use TS Box by georgewrmarshall · Pull Request #19949 · MetaMask/metamask-extension

georgewrmarshall · 2023-07-10T23:18:44Z

Explanation

Currently, the Text component is using the deprecated JavaScript version of the Box component. This blocks the TypeScript migration of the extension AND causes issues with the polymorphic as prop. This PR updates the Text component to use the TypeScript version of the Box and moves the old version of Text to text/deprecated folder and updates all imports across the extension.

Why not just update the `Text` component with `Box` if the API is the same?

I have another PR open doing just that but because of undiagnosable e2e test failures, I have resorted to this route. I will then subsequently update all import paths folder by folder to point to the updated component. This has proven to be a successful method in the past(thanks @brad-decker and @DDDDDanica 😁). It reduces the impact on engineers and merge conflicts and will help get to the bottom of the e2e test failures if they exist or if it's just an annoying timeout.

Fixes Update Text component to use TS version of Box #19569

This PR also unblocks 9 contributor PRs migrating components to TypeScript

Screenshots/Screencaps

Before

Text component in develop branch in storybook

before.mov

Deprecation message for deprecated Text component

After

Showing updated Text component in storybook using the TS version of the Box component. Showing no visual regressions and all stories and controls are working as expected. I have made some small updates to more easily find the replace Typography with Text section and linked the issue

after.mov

Manual Testing Steps

To check storybook

Go to the latest build of storybook in this PR
Search Text in the search panel
Check the controls, stories and docs are working as expected.

To check all import paths have been updated correctly.

The classNames of the 2 Text components are slightly different one uses the TypeScript Box and has the className mm-box vs the JavaScript version box-. SO this means that any snapshot tests will pick this up. There should be NO snapshot test updates with this update in this PR

I also did a search for <Text and part of the new import path text/deprecated to compare the number of files
165 files vs 164 files for .js files with the 1 difference being a comment in a deprecation message

11 files vs 11 files for .tsx files

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.

github-actions · 2023-07-10T23:18:57Z

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 · 2023-07-12T19:47:14Z

Adding deprecation notice to old version of Text component

metamaskbot · 2023-07-12T20:18:00Z

Builds ready [2e5a170]

builds: chrome, firefox
builds (beta): chrome
builds (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
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1608 ± 55 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	110	204	137	23	11
		domContentLoaded	1412	1887	1608	114	55
		load	1413	1887	1608	114	55
		domInteractive	1412	1887	1608	114	55

Bundle size diffs

background: 0 bytes
ui: 13275 bytes
common: 0 bytes

brad-decker

👏🏻 LGTM

metamaskbot · 2023-07-13T22:30:39Z

Builds ready [75cf2f6]

builds: chrome, firefox
builds (beta): chrome
builds (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
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1755 ± 109 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	109	368	175	65	31
		domContentLoaded	1406	2281	1754	226	109
		load	1406	2281	1755	226	109
		domInteractive	1406	2281	1754	226	109

Bundle size diffs

background: 0 bytes
ui: 13275 bytes
common: 0 bytes

metamaskbot · 2023-07-14T15:51:20Z

Builds ready [5213d0a]

builds: chrome, firefox
builds (beta): chrome
builds (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
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1527 ± 32 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	107	302	133	40	19
		domContentLoaded	1405	1695	1526	67	32
		load	1405	1695	1527	67	32
		domInteractive	1405	1695	1526	67	32

Bundle size diffs

background: 0 bytes
ui: 13275 bytes
common: 0 bytes

codecov · 2023-07-14T15:51:58Z

Codecov Report

Merging #19949 (5213d0a) into develop (91c8499) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop   #19949      +/-   ##
===========================================
- Coverage    69.47%   69.47%   -0.01%     
===========================================
  Files          988      988              
  Lines        37313    37306       -7     
  Branches      9989     9986       -3     
===========================================
- Hits         25923    25916       -7     
  Misses       11390    11390

Impacted Files	Coverage Δ
ui/components/app/add-network/add-network.js	`56.25% <ø> (ø)`
...nced-gas-fee-defaults/advanced-gas-fee-defaults.js	`100.00% <ø> (ø)`
...ed-gas-fee-gas-limit/advanced-gas-fee-gas-limit.js	`100.00% <ø> (ø)`
...s/app/approve-content-card/approve-content-card.js	`75.00% <ø> (ø)`
ui/components/app/beta-header/index.js	`100.00% <ø> (ø)`
...p/cancel-speedup-popover/cancel-speedup-popover.js	`78.12% <ø> (ø)`
ui/components/app/confirm-data/confirm-data.js	`93.75% <ø> (ø)`
...m-legacy-gas-display/confirm-legacy-gas-display.js	`93.55% <ø> (ø)`
.../components/app/confirm-hexdata/confirm-hexdata.js	`88.89% <ø> (ø)`
...page-container/confirm-page-container.component.js	`68.85% <ø> (ø)`
... and 145 more

FrederikBolding

LGTM

Note: I only reviewed the files under snaps ownership since this PR already had multiple approvals.

georgewrmarshall force-pushed the fix/19569/deprecate-old-text-add-new branch 4 times, most recently from 9ef3eb6 to 2e5a170 Compare July 12, 2023 19:45

georgewrmarshall commented Jul 12, 2023

View reviewed changes

georgewrmarshall marked this pull request as ready for review July 12, 2023 20:16

georgewrmarshall requested review from a team as code owners July 12, 2023 20:16

georgewrmarshall requested review from DDDDDanica, brad-decker, cortisiko, darkwing and garrettbear and removed request for cortisiko July 12, 2023 20:16

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

georgewrmarshall self-assigned this Jul 12, 2023

brad-decker previously approved these changes Jul 12, 2023

View reviewed changes

garrettbear previously approved these changes Jul 12, 2023

View reviewed changes

DDDDDanica previously approved these changes Jul 13, 2023

View reviewed changes

georgewrmarshall dismissed stale reviews from DDDDDanica, garrettbear, and brad-decker via b5ce5e1 July 13, 2023 21:50

georgewrmarshall force-pushed the fix/19569/deprecate-old-text-add-new branch from 2e5a170 to b5ce5e1 Compare July 13, 2023 21:50

georgewrmarshall force-pushed the fix/19569/deprecate-old-text-add-new branch from b5ce5e1 to 75cf2f6 Compare July 13, 2023 22:00

georgewrmarshall force-pushed the fix/19569/deprecate-old-text-add-new branch from 75cf2f6 to 858b0cb Compare July 13, 2023 23:17

Updating Text and import paths

5213d0a

georgewrmarshall force-pushed the fix/19569/deprecate-old-text-add-new branch from 858b0cb to 5213d0a Compare July 14, 2023 15:20

georgewrmarshall requested review from DDDDDanica, brad-decker and garrettbear July 14, 2023 17:00

garrettbear approved these changes Jul 14, 2023

View reviewed changes

DDDDDanica approved these changes Jul 14, 2023

View reviewed changes

brad-decker approved these changes Jul 14, 2023

View reviewed changes

FrederikBolding approved these changes Jul 14, 2023

View reviewed changes

georgewrmarshall merged commit 74cc312 into develop Jul 14, 2023

georgewrmarshall deleted the fix/19569/deprecate-old-text-add-new branch July 14, 2023 17:59

github-actions Bot locked and limited conversation to collaborators Jul 14, 2023

metamaskbot added the release-10.34.1 Issue or pull request that will be included in release 10.34.1 label Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating Text component to use TS Box#19949

Updating Text component to use TS Box#19949
georgewrmarshall merged 1 commit intodevelopfrom
fix/19569/deprecate-old-text-add-new

georgewrmarshall commented Jul 10, 2023 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 10, 2023

Uh oh!

georgewrmarshall Jul 12, 2023

Uh oh!

metamaskbot commented Jul 12, 2023

Uh oh!

brad-decker left a comment

Uh oh!

metamaskbot commented Jul 13, 2023

Uh oh!

metamaskbot commented Jul 14, 2023

Uh oh!

codecov Bot commented Jul 14, 2023 •

edited

Loading

Uh oh!

FrederikBolding left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

georgewrmarshall commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Why not just update the Text component with Box if the API is the same?

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions Bot commented Jul 10, 2023

Uh oh!

georgewrmarshall Jul 12, 2023

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Jul 12, 2023

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Jul 13, 2023

Uh oh!

metamaskbot commented Jul 14, 2023

Uh oh!

codecov Bot commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

FrederikBolding left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

georgewrmarshall commented Jul 10, 2023 •

edited

Loading

Why not just update the `Text` component with `Box` if the API is the same?

codecov Bot commented Jul 14, 2023 •

edited

Loading