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

perf: convert the "slide" svgs with embedded base64 pngs to just pngs #29333

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidmurdoch
Copy link
Contributor

@davidmurdoch davidmurdoch commented Dec 18, 2024

This PR needs a little more work, as the carousel where these images are used caches the image URLs into the metamask state the first time they are seen by the app. Updating the URLs (from svg to png) doesn't work, and actually breaks the images since the app will attempt to load the old svgs first.

The SVGs were 540.9KB, the PNGs are 193.8Kb.

"But wait", you probably interject. "SVGs are supposed to be better than PNGs for vector art!"

And you'd be right, but these SVGs aren't vector art; they are (mostly) just unoptimized PNGs embedded as a base64 encoded URL. The PNGs had some minor transforms, like rotation and squashing, applied, and in one instance, some weird blue angular shapes.

I maintained the spirit of the old SVGs in these new PNGs, but they aren't exactly pixel-perfect copies of the original SVGs.

"Why are these new PNGs so big now?" you might ask.

Well, that's just how big the embedded PNGs were inside the SVGs. I didn't enlarge anything. I kind of assume the creator of the SVGs knew the appropriate resolutions for the embedded images, and so I just rolled with that assumption (my hunch is that these images are just way too large for their intended use).

Description

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants