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

Landing optimizations #8030

Merged
merged 24 commits into from Nov 14, 2023
Merged

Conversation

shashkovdanil
Copy link
Contributor

@shashkovdanil shashkovdanil commented Nov 12, 2023

PR Type

Refactoring, Performance optimizations

Context

Closes #4657

Had issue bounty label?

  • Fill up your KSM address: Payout

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 Nuxt 3 prefetch all assets from every pages and every layouts. nuxt/nuxt#13778

@shashkovdanil shashkovdanil requested a review from a team as a code owner November 12, 2023 15:30
@shashkovdanil shashkovdanil requested review from preschian and Jarsen136 and removed request for a team November 12, 2023 15:30
Copy link

netlify bot commented Nov 12, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit ad2109c
🔍 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
Copy link
Collaborator

kodabot commented Nov 12, 2023

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!

@reviewpad reviewpad bot added large Pull request is large waiting-for-review labels Nov 12, 2023
@Gavin-Gong
Copy link
Contributor

Looks like the page has lost some CSS styles.

image

@roiLeo roiLeo added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Nov 13, 2023
@yangwao
Copy link
Member

yangwao commented Nov 13, 2023

Related as noticed adding @nuxt/image ?

@shashkovdanil
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

yangwao commented Nov 13, 2023

or maybe you have errors in the console?

current preview looks like this

image

@preschian
Copy link
Member

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

Copy link
Member

@preschian preschian left a comment

Choose a reason for hiding this comment

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

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

up

@shashkovdanil
Copy link
Contributor Author

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

@yangwao
Copy link
Member

yangwao commented Nov 13, 2023

I can help with SSR task

here was iirc some work done

@preschian
Copy link
Member

@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

Copy link
Member

@yangwao yangwao left a comment

Choose a reason for hiding this comment

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

I've noticed that #7924 loads on explorer items much faster than this PR, curious why

libs/ui/src/components/MediaItem/type/ImageMedia.vue Outdated Show resolved Hide resolved
@shashkovdanil
Copy link
Contributor Author

@preschian jfyi it's ready for merge

@prury
Copy link
Member

prury commented Nov 13, 2023

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

connect.wallet.mp4

Featured Articles:

image

fonts seems to be bold now for the navbar:

fonts.bolder.mp4

@prury prury requested a review from roiLeo November 13, 2023 18:12
@yangwao
Copy link
Member

yangwao commented Nov 13, 2023

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
Copy link
Member

prury commented Nov 13, 2023

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

Copy link

codeclimate bot commented Nov 13, 2023

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

View more on Code Climate.

Copy link

sonarcloud bot commented Nov 13, 2023

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
Copy link
Contributor Author

@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
Copy link
Member

prury commented Nov 14, 2023

@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

@prury prury removed the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Nov 14, 2023
@yangwao
Copy link
Member

yangwao commented Nov 14, 2023

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

@yangwao yangwao added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Nov 14, 2023
@shashkovdanil
Copy link
Contributor Author

shashkovdanil commented Nov 14, 2023

@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
Copy link
Contributor Author

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

Copy link
Contributor

@Jarsen136 Jarsen136 left a comment

Choose a reason for hiding this comment

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

code lgtm

@yangwao
Copy link
Member

yangwao commented Nov 14, 2023

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 yangwao added S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked and removed S-changes-requested-🤞 PR is almost good to go, just some fine tunning labels Nov 14, 2023
@yangwao
Copy link
Member

yangwao commented Nov 14, 2023

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

pay 80 usd

@yangwao yangwao merged commit 3670439 into kodadot:main Nov 14, 2023
16 checks passed
@yangwao
Copy link
Member

yangwao commented Nov 14, 2023

pay 80 usd

updated your address based on previous PR

@yangwao
Copy link
Member

yangwao commented Nov 14, 2023

😍 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

@yangwao yangwao added the paid pull-request has been paid label Nov 14, 2023
@roiLeo roiLeo mentioned this pull request Nov 15, 2023
1 task
This was referenced Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large paid pull-request has been paid S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Perf optimization on landing page
8 participants