kodadot / nft-gallery

Generative Art Marketplace
https://kodadot.xyz
MIT License
602 stars 347 forks source link

Landing optimizations #8030

Closed shashkovdanil closed 6 months ago

shashkovdanil commented 6 months ago

PR Type

Refactoring, Performance optimizations

Context

Closes #4657

Had issue bounty label?

What I've done

  1. Optimized fonts using @nuxtjs/google-fonts

Before (360kb):

font-before

After (50kb):

font-after
  1. Optimized image using @nuxt/image and converting png to webp, for example

Before (650kb):

blurred-before

After (3kb):

blurred-after
  1. Huge highlight.js to lightweight Prism

  2. Load huge @google/model-viewer only if the ModelMedia component is in the viewport

  3. Fixed lodash imports (only import someFn from 'lodash/someFn')

  4. Disabled prefetch for nuxt links. It's really big nuxt js problem: by default nuxt does prefetch for all pages and layouts, which is very bad for performance as we load a ton of unused JavaScript, currently there is no solution other than disabling prefetch for links manually. This problem is faced by many nuxt users, here is the issue https://github.com/nuxt/nuxt/issues/13778

netlify[bot] commented 6 months ago

Deploy Preview for koda-canary ready!

Name Link
Latest commit ad2109cabede3650c919fdc6134c08c6f90a50d2
Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/65529b131c11d90008529e69
Deploy Preview https://deploy-preview-8030--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.

kodabot commented 6 months ago

SUCCESS @shashkovdanil PR for issue #4657 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

Gavin-Gong commented 6 months ago

Looks like the page has lost some CSS styles.

image
yangwao commented 6 months ago

Related as noticed adding @nuxt/image ?

shashkovdanil commented 6 months ago

@Gavin-Gong please tell me how to reproduce it, or maybe you have errors in the console?

Screenshot 2023-11-13 at 11 01 31
shashkovdanil commented 6 months ago

Related as noticed adding @nuxt/image ?

@yangwao ye it's related, I think we can keep my version, I've tested it in different browsers, also configured caching

yangwao commented 6 months ago

or maybe you have errors in the console?

current preview looks like this

image

preschian commented 6 months ago

tbh, I would prefer to revisit the issue after SSR is supported

shashkovdanil commented 6 months ago

@preschian I can help with SSR task if you don't mind. I've had a lot of experience with it

yangwao commented 6 months ago

I can help with SSR task

here was iirc some work done

preschian commented 6 months ago

@shashkovdanil you can keep these improvements if you want:

1. Optimized fonts using @nuxtjs/google-fonts
3. Huge highlight.js to lightweight Prism
4. Load huge @google/model-viewer only if the ModelMedia component is in the viewport
5. Fixed lodash imports (only import someFn from 'lodash/someFn')

but please, skip prefetch stuff for now

shashkovdanil commented 6 months ago

@shashkovdanil you can keep these improvements if you want:

1. Optimized fonts using @nuxtjs/google-fonts
3. Huge highlight.js to lightweight Prism
4. Load huge @google/model-viewer only if the ModelMedia component is in the viewport
5. Fixed lodash imports (only import someFn from 'lodash/someFn')

but please, skip prefetch stuff for now

@preschian okay, ye, removing prefetch is risky, so I did it in a separate commit, okay, I'll revert it.

shashkovdanil commented 6 months ago

@shashkovdanil you can keep these improvements if you want:

1. Optimized fonts using @nuxtjs/google-fonts
3. Huge highlight.js to lightweight Prism
4. Load huge @google/model-viewer only if the ModelMedia component is in the viewport
5. Fixed lodash imports (only import someFn from 'lodash/someFn')

but please, skip prefetch stuff for now

Done!

yangwao commented 6 months ago

We've lost some accessibility, best practices and SEO points apparently in this PR πŸ‘€

canary.kodadot.xyz image

this PR image

shashkovdanil commented 6 months ago

We've lost some accessibility, best practices and SEO points apparently in this PR πŸ‘€

canary.kodadot.xyz image

this PR image

@yangwao It won't affect SEO in any way. The thing is that before the images for Article were made through div with background-image, I changed them to img with loading="lazy", but did not add alt attribute, because of this the ranking decreased. Now I will add alt

prury commented 6 months ago

search test failing 😒

yangwao commented 6 months ago

Now I will add alt

image works!

shashkovdanil commented 6 months ago

@prury @preschian I need advice, search.spec crashes because there is this code Array.from(document.images).every((i) => i.complete), which checks if ALL images from the page are loaded. But since loading="lazy" was added to improve performance, only those images that are in the viewport are loaded.

What's the best way to fix the test? Why does the test wait for all images to load? Can this be removed?

preschian commented 6 months ago

But since loading="lazy"

I prefer to remove lazy on libs/ui/src/components/MediaItem/type/ImageMedia.vue and libs/ui/src/components/MediaItem/type/ModelMedia.vue. Because the component can be rendered on above the fold section https://web.dev/articles/lazy-loading-best-practices#:~:text=Anything%20resting%20above%20the%20fold,finished%20loading%20and%20begin%20execution

We can put loading="lazy" if we are sure that the component is below the fold

shashkovdanil commented 6 months ago

Ok, I removed loading="lazy", we can think later how to implement it more intelligently

shashkovdanil commented 6 months ago

@preschian jfyi it's ready for merge

prury commented 6 months ago

Talisman wallet image won't load the first time you try to connect wallet on desktop:

https://github.com/kodadot/nft-gallery/assets/36627808/500e8835-d97c-4156-a15a-be78254894fa

Featured Articles:

image

fonts seems to be bold now for the navbar:

https://github.com/kodadot/nft-gallery/assets/36627808/d7428a22-1d76-46ea-b682-dd95c0cff3c5

yangwao commented 6 months ago

Seems fonts has changed as @prury reported

canary image pr image

Talisman wallet image won't load the first time you try to connect wallet on desktop:

For first loads I wasn't able connect pjs

prury commented 6 months ago

@shashkovdanil sorry for the trouble, but connect wallet problem persists

codeclimate[bot] commented 6 months ago

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

View more on Code Climate.

sonarcloud[bot] commented 6 months ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

shashkovdanil commented 6 months ago

@prury ufff it was crazy bug. If you import the chain function as import { chain } from 'lodash', everything works fine. But then we're downloading 200kb of lodash. If we import this function correctly, using import chain from 'lodash/chain', then on the first call we get the error that chain(...).map is not function. I just rewrote the code without using the chain function

prury commented 6 months ago

@prury ufff it was crazy bug. If you import the chain function as import { chain } from 'lodash', everything works fine. But then we're downloading 200kb of lodash. If we import this function correctly, using import chain from 'lodash/chain', then on the first call we get the error that chain(...).map is not function. I just rewrote the code without using the chain function

oh πŸ˜„, thank you for rewriting, wallet bug is now gone

yangwao commented 6 months ago

Great, we've gained in performance, yet it seems we've lost some points in best practices, you might have a look @shashkovdanil, please ?

image

shashkovdanil commented 6 months ago

@yangwao, I did a check and got this result. Deviations of 5-7 units are more of a margin of error. In this PR, I didn't change anything SEO or a11y related

image

shashkovdanil commented 6 months ago

By the way, I can help improve the accessibility of the site in the future. Fix keyboard navigation, put a11y attributes everywhere. If you have a request for this

yangwao commented 6 months ago

Fix keyboard navigation, put a11y attributes everywhere. If you have a request for this

Sure, you can make follow-up!

I think primarily performance would help keep it on green currently LCP, CLS and so on

yangwao commented 6 months ago

Did not notice anything broken, let's see, merging it, great PR @shashkovdanil thanks!

pay 80 usd

yangwao commented 6 months ago

pay 80 usd

updated your address based on previous PR

yangwao commented 6 months ago

😍 Perfect, I’ve sent the payout πŸ’΅ $80 @ 5.44 USD/DOT ~ 14.706 $DOT πŸ§— 16SpvdDgKNiQ3c53DxX7refnQcKUD3uRNim3Z1HBJLNGrtSP πŸ”— 0xded7066fa6caa4986584a1dc2ed7cf9703fe27c2efd591b906ba137693a4e312

πŸͺ… Let’s grab another issue and get rewarded! πŸͺ„ github.com/kodadot/nft-gallery/issues