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: collection/gallery explore page images #10443

Merged
merged 19 commits into from
Jul 18, 2024
Merged

Conversation

hassnian
Copy link
Contributor

@hassnian hassnian commented Jun 10, 2024

PR Type

  • Bugfix
  • Feature
  • Refactoring

Context

First pr of more to come to close the main issue, this pr only focuses on the explore pages
mainly

  • lazy loading of images
  • use cfi size variants to load smaller images where possible

Collection Explore

  • lazy load images: eagerly load first 3 rows of each fetch remaining are lazy loaded
    • first page load (image resources)
      - mobile: 2.5mb (this pr) vs 19.8mb (prod)
      - desktop: 3mb (this pr) vs 19.8mb (prod)
  • use metadata from collection meta , saves an extra call, most collections didn't need that (ahp 87/94 collections),
  • use cfi size variants in collection images
    • banner: about the same for me , but should fetch smaller images depending on the user's display
    • image: average of 8-10x smaller images
  • profile image: use cfi size variants
  • remove: pwa.client.ts

Items Explore

Needs QA check

  • @kodadot/qa-guild please review

Did your issue had any of the "$" label on it?

  • Fill up your DOT address: Payout

Screenshot 📸

  • My fix has changed something on UI;

Copy link

netlify bot commented Jun 10, 2024

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit eb663f0
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/669900c0b8a62d00086131ab
😎 Deploy Preview https://deploy-preview-10443--koda-canary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

socket-security bot commented Jun 14, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher

View full report↗︎

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@hassnian hassnian changed the title optimization: Request heavy app - high amount of fetches optimization: collection/gallery explore page images Jun 15, 2024
@hassnian hassnian changed the title optimization: collection/gallery explore page images perf: collection/gallery explore page images Jun 15, 2024
@hassnian hassnian marked this pull request as ready for review June 15, 2024 04:42
@hassnian hassnian requested a review from a team as a code owner June 15, 2024 04:42
@hassnian hassnian requested review from daiagi, Jarsen136 and preschian and removed request for a team and daiagi June 15, 2024 04:42
@hassnian
Copy link
Contributor Author

hassnian commented Jun 15, 2024

@preschian should we remove providers/cloudflare.ts, and route all images through the image worker ?

CleanShot 2024-06-15 at 10 11 00@2x

also some images fail to load like

cfi ❌ https://kodadot.xyz/cdn-cgi/imagedelivery/jk5b6spi_m_-9qC4VTnjpg/bafybeiaq2lvwomxmtmqyr5yigi4mty7ybib4myqewv6c265aheqv6lvjgu/w=700

image worker ✅ https://image-beta.w.kodadot.xyz/ipfs/bafybeiaq2lvwomxmtmqyr5yigi4mty7ybib4myqewv6c265aheqv6lvjgu

@preschian
Copy link
Member

should we remove providers/cloudflare.ts, and route all images through the image worker ?

Yes, this one can help reduce the request counts. But we need to implement cfi size variants in image-worker first

  • use metadata from collection meta , saves an extra call, most collections didn't need that (ahp 87/94 collections),

if the meta = null, try to get the image from metadata

@prury
Copy link
Member

prury commented Jun 17, 2024

i'm having some problems when scrolling down images on ahp, at some point it takes a long time to be loaded(here i'm on page 3):

image

could be unrelated as we are having some problems on ahp

@hassnian
Copy link
Contributor Author

hassnian commented Jun 21, 2024

Yes, this one can help reduce the request counts. But we need to implement cfi size variants in image-worker first

done, needs worker update kodadot/workers#313

i'm having some problems when scrolling down images on ahp, at some point it takes a long time to be loaded(here i'm on page 3):

could be unrelated as we are having some problems on ahp

worked fine for me, @prury let's try again after kodadot/workers#313 is merged

@prury prury added the S-external-resource-needed PR needs a worker to be merged/deployed, a enviroment variable, a API key or any external resource label Jun 21, 2024
@hassnian
Copy link
Contributor Author

@prury the worker is ready, try again and let me know

components/collection/CollectionCard.vue Show resolved Hide resolved
plugins/pwa.client.ts Outdated Show resolved Hide resolved
@prury prury added S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked waiting-for-review and removed S-external-resource-needed PR needs a worker to be merged/deployed, a enviroment variable, a API key or any external resource labels Jun 26, 2024
@vikiival
Copy link
Member

vikiival commented Jul 3, 2024

cc @Jarsen136 pls review
cc @hassnian pls resolve conflicts

@hassnian
Copy link
Contributor Author

hassnian commented Jul 9, 2024

cc @hassnian pls resolve conflicts

done

@hassnian hassnian added S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved and removed waiting-for-review labels Jul 11, 2024
@vikiival
Copy link
Member

same here

vikiival added the S-resolve-merge-conflicts-🤝 label now

@hassnian
Copy link
Contributor Author

same here

vikiival added the S-resolve-merge-conflicts-🤝 label now

done

@vikiival
Copy link
Member

One test is failing @hassnian

Copy link

codeclimate bot commented Jul 18, 2024

Code Climate has analyzed commit eb663f0 and detected 0 issues on this pull request.

View more on Code Climate.

@hassnian
Copy link
Contributor Author

One test is failing @hassnian

@vikiival all tests are passing

@vikiival vikiival added this pull request to the merge queue Jul 18, 2024
Merged via the queue into kodadot:main with commit 2afd840 Jul 18, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved S-resolve-merge-conflicts-🤝 S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants