rainbow icon indicating copy to clipboard operation
rainbow copied to clipboard

Published 20 hours ago • verified publisher icon rainbow-me

Improvements to assets transformations

Open osdnk opened this issue 2 years ago • 6 comments

UPDATE RAINBOW SCRIPTS BEFORE TESTING TO AVOID CACHING ISSUES

This PR creates a bunch of improvements to our Images patterns. I don't include a ticket for this cause all of them are results of observations we have from https://www.notion.so/rainbowdotme/Jarich-Micha-imgix-bullshit-cccbcbb14a074336a42593c4419ae1d9

and I was just fixing them when noticing what is wrong.

I made a bunch of improvements:

  1. ImgIxImage is now TransformationImage. This makes much more sense because we also use a bit of cloudinary logic there when we do something related to assets (introduced in https://github.com/rainbow-me/rainbow/pull/3490).
  2. When no options are applied (and the domain is considered safe), we skip signing and use the URL as is. If we don't do anything, we can just skip it. We can do this on a higher level, but I believe the change above is actually hiding a bit of abstraction, so we might also hide this.
  3. in src/components/unique-token/UniqueTokenImage.js we remove the transformation to png in ImageTile that is already accepting lowResUrl and imageUrl that is png (via svgtoPngIfNeeded used in src/parsers/uniqueTokens.js:52)
  4. We don't use signing and imgix stuff if we need only to change the size of googleusercontent images. This is enough to just change the size in the URL.

This should reduce our ImgIx usage by about 40-60%

no changes observable

Dev checklist for QA: what to test

  • firstly run the app in develop, then switch to this. This is important to test if the migration works correctly, because we need to be sure that imgix cache will not return old data

Test all NFTs. Particularly focus on the things you see on the wallet screen and in an expanded state. Dame.eth is a good wallet. test:

  • Normal NFTs
  • Poaps
  • Uniswap positions (animated SVGs)
  • shields (non-animated svgs)

osdnk avatar Jun 20 '22 19:06 osdnk

@tchayen the first thing is because you had not updated the internals. The second is because of the ImgIx cache that I migrated now.

osdnk avatar Jun 23 '22 10:06 osdnk

Tried with non-empty wallet on Pixel 6 and Xiaomi Redmi, worked for me the images were there

jkadamczyk avatar Jun 23 '22 11:06 jkadamczyk

Hey @estebanmino we like it or not, this is what i see on develop as well. But thanks for pointing it out!

osdnk avatar Jun 29 '22 11:06 osdnk

/preview

BrodyHughes avatar Jul 05 '22 15:07 BrodyHughes

Messaging Michal about a couple things on this.

BrodyHughes avatar Jul 07 '22 17:07 BrodyHughes

@osdnk is this PR worth saving? Or should we close, re-scope and prioritize it for future work?

estrattonbailey avatar Sep 21 '22 16:09 estrattonbailey

I'm closing this, since we lack too much context about all this. If we think we should still pursue merging it and getting it over the finish line we can re-open this PR and rebase it

jkadamczyk avatar Oct 17 '22 20:10 jkadamczyk