Skip to content
This repository has been archived by the owner on Jun 2, 2022. It is now read-only.

chore: Performance improvements by sections #368

Merged
merged 6 commits into from
Mar 4, 2022
Merged

chore: Performance improvements by sections #368

merged 6 commits into from
Mar 4, 2022

Conversation

tlgimenes
Copy link
Contributor

@tlgimenes tlgimenes commented Mar 3, 2022 •
edited
Loading

What's the purpose of this pull request?

This PR uses the new content-visibility css property to improve layout loading of the page.

How does it work?

We organize our page layouts in sections. These sections are self contained strips of the page that offers a feature (carousel, shelf, hero etc).
Turns out, that for such architecture, W3C has recently launched a new CSS feature to lazy load part of the page's content so browsers do less work. This feature is called content-visibility and better explanation is done in web.dev's site

To improve our sections architecture, I created a new Section component that encapsulates this css rules with the section selector.

Below, you can see, with 95% confidence interval, rendering times for the home page:

before: 396 +- 18
after: 328 +- 11

which is ~20% improvement in rendering times of the home page. This is a small improvement, however, given the small effort, I guess we can move forward with this PR

How to test it?

Nothing should have changed

Checklist

  • CHANGELOG entry added

@netlify
Copy link

netlify bot commented Mar 3, 2022 •
edited
Loading

✔️ Deploy Preview for basestore ready!

🔨 Explore the source changes: 1ede4b8

🔍 Inspect the deploy log: https://app.netlify.com/sites/basestore/deploys/62224f8ccf5795000763a427

😎 Browse the preview: https://deploy-preview-368--basestore.netlify.app

@vtex-sites
Copy link

vtex-sites bot commented Mar 3, 2022 •
edited
Loading

Preview is ready

This pull request generated a Preview

👀   Preview: https://preview-368--base.preview.vtex.app
🔬   Go deeper by inspecting the Build Logs
📝   based on commit dd8df71

@gatsby-cloud
Copy link

gatsby-cloud bot commented Mar 3, 2022 •
edited
Loading

Gatsby Cloud Build Report

basestore

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 4m

Performance

Lighthouse report

Metric Score
Performance 💚 92
Accessibility 💚 100
Best Practices 💚 100
SEO 💚 93

🔗 View full report

@tlgimenes tlgimenes force-pushed the chore/content-visibility branch 2 times, most recently from f7aa56a to 8287b0a Compare March 3, 2022 16:40
@tlgimenes tlgimenes marked this pull request as ready for review March 3, 2022 20:00
@tlgimenes tlgimenes force-pushed the chore/content-visibility branch from a794445 to 28a6db5 Compare March 3, 2022 20:30
filipewl
filipewl reviewed Mar 4, 2022
View reviewed changes
Copy link
Contributor

@filipewl filipewl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Really cool CSS feature I didn't know much about. I learned a lot reading the web.dev link and other resources (W3C)! I was initially very confused trying to differentiate contain, content-visibility, and contain-intrinsic-size 😵. Interesting to see CSS Containment has been here since Chrome 52, but only now seems to be getting traction. Maybe because there were some accessibility concerns that only got addressed in 85-89 and improvements in 98.

Left some minor suggestions and will re-review after reading your thoughts on them!

src/components/sections/Section/Section.scss Outdated Show resolved Hide resolved
src/components/sections/Section/Section.scss Outdated Show resolved Hide resolved
@filipewl
Copy link
Contributor

filipewl commented Mar 4, 2022 •
edited
Loading

By the way, merging master should fix the CI issues, addressed in #369!

@tlgimenes tlgimenes force-pushed the chore/content-visibility branch from 28a6db5 to bc82933 Compare March 4, 2022 13:51
hellofanny
hellofanny approved these changes Mar 4, 2022
View reviewed changes
Copy link
Contributor

@hellofanny hellofanny left a comment

Choose a reason for hiding this comment

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

Same thoughts as @filipewl! I didn't know much about it and took the chance to learn more. Thanks for the explanation! 🤩

eduardoformiga
eduardoformiga reviewed Mar 4, 2022
View reviewed changes
Copy link
Member

@eduardoformiga eduardoformiga left a comment

Choose a reason for hiding this comment

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

Really nice to learn more about these CSS props! thanks for bringing it up. 🎉

I left some comments just for the sake of understanding better the things

src/components/sections/Section/Section.scss Outdated Show resolved Hide resolved
src/components/sections/Section/Section.tsx Outdated Show resolved Hide resolved
src/pages/index.tsx Outdated Show resolved Hide resolved
@tlgimenes tlgimenes added the performance Improvements to performance label Mar 4, 2022
filipewl
filipewl approved these changes Mar 4, 2022
View reviewed changes
Copy link
Contributor

@filipewl filipewl left a comment

Choose a reason for hiding this comment

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

🚀

@tlgimenes tlgimenes merged commit dd8df71 into master Mar 4, 2022
@tlgimenes tlgimenes deleted the chore/content-visibility branch March 4, 2022 18:08
@eduardoformiga eduardoformiga mentioned this pull request Oct 6, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hellofanny hellofanny hellofanny approved these changes

@filipewl filipewl filipewl approved these changes

@eduardoformiga eduardoformiga Awaiting requested review from eduardoformiga

Assignees
No one assigned
Labels
performance Improvements to performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tlgimenes @filipewl @hellofanny @eduardoformiga