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

updated images and videos for better browser loading #1158

Closed
wants to merge 6 commits into from

Conversation

saisuvanth
Copy link

@saisuvanth saisuvanth commented May 9, 2023

Description
Regarding the lighthouse issue, I have updated all images to webp and some videos to webm which are blocking the page rendering. Also achieved a significant improvement in lighthouse score ( The overall score is low because of not following best practices such as https in localhost). Score will increase in production build, because of caching and HTTPS usage.

This PR fixes #973

Before

j

After

h

Notes for Reviewers

Can you please let me know how the website is deploying at production build. The overhead here is because of size of CSS and JS files which need browser caching configuration from serving side.

Signed commits

  • Yes, I signed my commits.

@welcome
Copy link

welcome bot commented May 9, 2023

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Newcomers' Guide and sure to join the community Slack.
Be sure to double-check that you have signed your commits. Here are instructions for making signing an implicit activity while peforming a commit.

@netlify
Copy link

netlify bot commented May 9, 2023

Deploy Preview for mesheryio-preview ready!

Name Link
🔨 Latest commit 532d3a6
🔍 Latest deploy log https://app.netlify.com/sites/mesheryio-preview/deploys/649afd591f4d7f0008c04bc1
😎 Deploy Preview https://deploy-preview-1158--mesheryio-preview.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.

@github-actions github-actions bot added area/blog area/catalog area/ci Continuous integration | Build and release area/docs Improvements or additions to documentation area/website framework/helm labels May 9, 2023
@vishalvivekm
Copy link
Member

@saisuvanth let's discuss this on websites call, add this as an agenda item in the meeting minutes , if you would :)

@Shivam-AfA
Copy link
Contributor

Woah! Great work @saisuvanth This is something really impactful that you have worked on here. Thanks for this.

@leecalcote
Copy link
Member

merge conflicts...

@leecalcote
Copy link
Member

@ayushthe1 please help move this PR forward.

Copy link
Contributor

@ayushthe1 ayushthe1 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @saisuvanth .Could you also add loading = lazy to img tags where it's not been used. Also did you manually converted all the images to webp or used some sort of script ?

@saisuvanth
Copy link
Author

Yeah okay will do that, and yeah I wrote a bash script for image conversion.

@vishalvivekm
Copy link
Member

Yeah okay will do that, and yeah I wrote a bash script for image conversion.

any update @saisuvanth ?

@saisuvanth
Copy link
Author

@vishalvivekm yep updated all images to load lazily. You can merge the branch safely.
Also I want to do some research on this issue for further improvement.

Copy link
Contributor

@ayushthe1 ayushthe1 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @saisuvanth 👍

@thisiskaransgit
Copy link
Contributor

@saisuvanth, thank you for working on this, due to the scope of the changes, reviewing got delayed, kindly resolve the merge conflicts, so that we can move further.

@saisuvanth
Copy link
Author

@thisiskaransgit Hey, can you check once I have merged and pushed into the same branch.

@Shivam-AfA
Copy link
Contributor

@saisuvanth Still merge conflicts....

@@ -29,11 +29,11 @@
{% for sublink in link.subitems %}
<li >
<div {% if sublink.link == page.url %} class="sub-nav-li sub-nav-li--active" {% else %} class="sub-nav-li" {% endif %}>
<img class="nav-img" src="{{sublink.img_src}}">
<img class="nav-img" loading="lazy" src="{{sublink.img_src}}">

Choose a reason for hiding this comment

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

I don't think we should not lazy load navigation because it is directly visible to user when the page loads

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Okay I'll change that into normal loading
any fixes other than that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@saisuvanth, resolve the conflicts please

@Chadha93
Copy link
Member

@saisuvanth Any update here? Let's close this PR, given the unaddressed feedback and the number of merge conflicts. Feel free to open new PR.

@Chadha93 Chadha93 closed this Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/blog area/catalog area/ci Continuous integration | Build and release area/docs Improvements or additions to documentation area/website component/integrations framework/helm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Low Lighthouse score of Meshery.io
8 participants