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.

Sign up for GitHub

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

Jump to bottom

chore: Optimize/10528 remove generated svgs #10535

Merged
merged 5 commits into from
Aug 2, 2024
Merged

chore: Optimize/10528 remove generated svgs #10535

merged 5 commits into from
Aug 2, 2024

Conversation

Cal-L
Copy link
Contributor

@Cal-L Cal-L commented Aug 2, 2024 •
edited
Loading

Description

These changes removes the generated SVGs for contract-metadata. In addition to it taking ~7MB of the JS bundle size, it was also most likely no longer used.

Related issues

Fixes:
#10528

Manual testing steps

Navigate around where tokens appear and ensure that the tokens still render properly. Some areas to test:

  • Swaps token search
  • Adding tokens to wallet
  • Detected tokens
  • Token details

Screenshots/Recordings

Before

JS bundle size on main
image

After

JS bundle size after removal
image

Spot check
https://github.com/user-attachments/assets/83579c20-82d9-45c0-bf9a-826cebce18df

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.

@Cal-L Cal-L requested a review from a team as a code owner August 2, 2024 01:20
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 2, 2024

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.

@Cal-L Cal-L added No QA Needed Apply this label when your PR does not need any QA effort. Run Smoke E2E Triggers smoke e2e on Bitrise needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Spot Check on the Release Build If a ticket doesn't require feature QA, but does require some form of manual spot checking labels Aug 2, 2024
@Cal-L Cal-L linked an issue Aug 2, 2024 that may be closed by this pull request
Remove generated SVGs in contract-metadata #10528
Closed
9 tasks
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 2, 2024 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: b2842ef
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/813dbb7b-56d9-4ec2-a9a8-c27ad36d1212

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@Cal-L Cal-L removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Aug 2, 2024
@Cal-L
Copy link
Contributor Author

Cal-L commented Aug 2, 2024

E2E passed on last commit with functional changes

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Aug 2, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
66.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@Cal-L
Copy link
Contributor Author

Cal-L commented Aug 2, 2024

SVGs are not commited to the repo and is generated during a release build. The SVGs will not longer be generated

@Cal-L Cal-L merged commit 0826e24 into main Aug 2, 2024
33 checks passed
@Cal-L Cal-L deleted the optimize/10528-remove-generated-svgs branch August 2, 2024 18:34
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 2, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2024
@metamaskbot metamaskbot added the release-7.30.0 Issue or pull request that will be included in release 7.30.0 label Aug 2, 2024
NicolasMassart
NicolasMassart reviewed Aug 2, 2024
View reviewed changes
Copy link
Contributor

@NicolasMassart NicolasMassart left a comment

Choose a reason for hiding this comment

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

tracking usage of this assetIcon I found app/components/UI/AssetList/index.js that doesn't seem to be used either. Dead code or mistake?

@Cal-L
Copy link
Contributor Author

Cal-L commented Aug 12, 2024

@NicolasMassart I believe the logic of contract metadata within AssetIcon is dead code, which was removed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NicolasMassart NicolasMassart NicolasMassart left review comments

@tommasini tommasini tommasini approved these changes

Assignees
No one assigned
Labels
No QA Needed Apply this label when your PR does not need any QA effort. release-7.30.0 Issue or pull request that will be included in release 7.30.0 Spot Check on the Release Build If a ticket doesn't require feature QA, but does require some form of manual spot checking team-mobile-platform
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove generated SVGs in contract-metadata
4 participants
@Cal-L @NicolasMassart @tommasini @metamaskbot