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

chore: remove MMI UI code #29884

Merged
merged 7 commits into from
Jan 29, 2025
Merged

chore: remove MMI UI code #29884

merged 7 commits into from
Jan 29, 2025

Conversation

shane-t
Copy link
Member

@shane-t shane-t commented Jan 23, 2025

Description

As part of the MMI deprecation and code removal, it's necessary to remove the fenced UI code before the background code, since the UI code is lower in the dependency chain.

This PR removes any code that's inside a @build-mmi code fence. In order to satisfy various requirements this also means removing unused imports, which the PR also deletes.

The PR also removes tests (which have never been code fenced) because in many cases they are caused by fenced code.

In addition, any non-fenced code which is known to be MMI specific is removed.

It does not remove inverse MMI code fences, i.e. fences which are there to stop things running in MMI and are now redundant. This is for the sake of keeping the PR a bit smaller. Those will be removed in the next PR.

Related issues

Fixes: #29782

Manual testing steps

Not applicable, as no new feature is added. However, many/most UI features are touched.

Screenshots/Recordings

Before

image

After

image

Pre-merge author checklist

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.

@metamaskbot metamaskbot added the team-mmi PRs from the MMI team label Jan 23, 2025
@shane-t shane-t force-pushed the 29782-remove-mmi-ui-code branch 6 times, most recently from 9d8b59d to b470fab Compare January 24, 2025 01:10
@metamaskbot
Copy link
Collaborator

Builds ready [9d8b59d]
Page Load Metrics (2066 ± 269 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint140632152073565272
domContentLoaded139830552032536257
load140630992066559269
domInteractive24164654923
backgroundConnect11131473517
firstReactRender1695432411
getState578222211
initialActions00000
loadScripts100323611531456219
setupStore65424189
uiStartup161936462388640307
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -120 Bytes (-0.00%)
  • ui: -9.18 KiB (-0.12%)
  • common: -341 Bytes (-0.00%)

@shane-t shane-t mentioned this pull request Jan 23, 2025
9 tasks
@shane-t shane-t force-pushed the 29782-remove-mmi-ui-code branch from d7d5042 to 749bfb1 Compare January 24, 2025 01:37
@shane-t shane-t changed the title chore: remove MMI fenced UI code Remove MMI UI code Jan 24, 2025
@shane-t shane-t marked this pull request as ready for review January 24, 2025 01:45
@shane-t shane-t requested review from a team as code owners January 24, 2025 01:45
@shane-t shane-t changed the title Remove MMI UI code chore: remove MMI UI code Jan 24, 2025
@@ -61,6 +61,7 @@ export enum IconName {
Connect = 'connect',
CopySuccess = 'copy-success',
Copy = 'copy',
Custody = 'custody',
Copy link
Member Author

Choose a reason for hiding this comment

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

The snaps SDK exports a type that contains this, so we can't remove this without causing typescript errors

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also an instance of this being used in a test that has not been removed

Screenshot 2025-01-28 at 11 26 36 AM

@metamaskbot
Copy link
Collaborator

Builds ready [749bfb1]
Page Load Metrics (1869 ± 86 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24124071792398191
domContentLoaded16602382183118488
load16822399186918086
domInteractive26102422211
backgroundConnect16139503015
firstReactRender1695432813
getState775272210
initialActions01000
loadScripts11711874134517584
setupStore779192010
uiStartup196126852177220106
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -9.18 KiB (-0.12%)
  • common: -5.63 KiB (-0.06%)

@metamaskbot
Copy link
Collaborator

Builds ready [c407b74]
Page Load Metrics (1688 ± 91 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27321541614359172
domContentLoaded14352129165917383
load14792199168818991
domInteractive24108382311
backgroundConnect895302411
firstReactRender1694452813
getState490202412
initialActions01000
loadScripts10151648119014771
setupStore781182110
uiStartup167126511972298143
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -9.15 KiB (-0.11%)
  • common: -5.63 KiB (-0.06%)

@metamaskbot
Copy link
Collaborator

Builds ready [f21a6ad]
Page Load Metrics (1723 ± 72 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14982079172415474
domContentLoaded14882008169214067
load15012031172315072
domInteractive219838189
backgroundConnect875302110
firstReactRender1678302010
getState567212010
initialActions00000
loadScripts10241525122813565
setupStore86521199
uiStartup16882373196716881
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -9.15 KiB (-0.11%)
  • common: -5.63 KiB (-0.06%)

@metamaskbot
Copy link
Collaborator

Builds ready [3358806]
Page Load Metrics (1904 ± 56 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint23421481744496238
domContentLoaded17122064187911656
load17242093190411656
domInteractive25168533215
backgroundConnect107225189
firstReactRender1699422914
getState563282211
initialActions01000
loadScripts11891609138811153
setupStore889262613
uiStartup197427792213217104
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -9.15 KiB (-0.11%)
  • common: -5.63 KiB (-0.06%)

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 comment. Also there are some unused CSS classes that still need to be removed. When searching for institutional in the ui/ folder it shouldn't block this PR though

Screenshot 2025-01-28 at 11 24 15 AM

@@ -61,6 +61,7 @@ export enum IconName {
Connect = 'connect',
CopySuccess = 'copy-success',
Copy = 'copy',
Custody = 'custody',
Copy link
Contributor

Choose a reason for hiding this comment

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

There is also an instance of this being used in a test that has not been removed

Screenshot 2025-01-28 at 11 26 36 AM

Copy link
Member

@NidhiKJha NidhiKJha left a comment

Choose a reason for hiding this comment

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

LGTM! Verified the componenents in multichain directory

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we need to make the same deletions in en_GB

Copy link
Member

Choose a reason for hiding this comment

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

Our linter script must not consider en_GB. Consider this as non-blocking, we can fix that separately. There may be other discrepencies with that locale as well, since the linter misses it.

Copy link
Member

Choose a reason for hiding this comment

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

Tracked here: #29965

Copy link
Contributor

@sleepytanya sleepytanya left a comment

Choose a reason for hiding this comment

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

Reviewed Confirmations directories.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM! As George highlighted, there are a couple more custody/institutional references in UI tests/mock data/stypes, but we can remove those separately

@Gudahtt Gudahtt added this pull request to the merge queue Jan 29, 2025
Merged via the queue into main with commit 08365f0 Jan 29, 2025
70 checks passed
@Gudahtt Gudahtt deleted the 29782-remove-mmi-ui-code branch January 29, 2025 14:46
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2025
@metamaskbot metamaskbot added the release-12.12.0 Issue or pull request that will be included in release 12.12.0 label Jan 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.12.0 Issue or pull request that will be included in release 12.12.0 team-mmi PRs from the MMI team
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants