-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[website] Improve rebranding homepage performance #27838
Conversation
2cd2a77
to
52a4f6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lazy load each section below the fold using react-intersection-observer
Could we only lazy load the heavy sections that are not relevant for SEO? For instance, if the approach means that Google bot can't extract the relevant content of the page and index it, I think that it's K.O.
@@ -1,98 +1,27 @@ | |||
import * as React from 'react'; | |||
import dynamic from 'next/dynamic'; | |||
import { useInView } from 'react-intersection-observer'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving this to the lab, so we can dog food #16663?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why, I don't get your point? Can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper seems simple enough for us to implement a quick & dirty version and start experimenting with it before we expose something public.
The areas that are lazy loading. Do you think it will have an impact on SEO? (edited) sponsors section is not lazy load anymore. |
@siriwatknp More than SEO, I'm worried about two other things:
I would propose we use this viewport technique as little as possible, at least, never at the expense of a changing scroll height. |
Regarding pushing it one step futher. The initial request is suspicious. Compare: Maybe it's the size of the HTML output. Should we shorten the class names in production? From |
@oliviertassinari Can you try again? I forgot to add placeholder when it is loading. Tested with poor connection, I don't see obvious layout shift.
In the context of rebranding, our goal is not to build API for solving landing page perf. So, I think it is not relevant.
fixed. I think it is a trade-off to decide which one we can live with. Here is the data I get from google analytics, the past year However, the code looks a bit more complicated and the Placeholder's height need to be calculated manually to prevent layout shift which means if design change in the future, the height might change. The question is "Can we live with this?". For me, the total time reduced in a year is far better than the increase of code complexity and maintenance (I don't think we will change the homepage design more than once a year). Moreover, it is not a public API so we can revert back at anytime if we found out it is not worth by whatever reason. Anyway, waiting to see what other team members's thought. |
This reverts commit 5c3b051.
4156b5b
to
1151527
Compare
No layout shifts now, even on the crappy shared WiFi of my hotel. Niice.
I would have framed the question as: "Are we OK dodging a performance root issue on the homepage for improving developers's perception?" To be fair, we were already doing it in the homepage of v4 with NoSsr. |
I like the approach if it's only focused on non-content e.g. images, component demos. But you're also lazy-loading sponsors which I wouldn't do. Part of their interest is to get exposure. Lazy-loading them diminishes that significantly.
Who said that? The landing page is the first impression for our product. We can't just neglect that.
This is not a tradeoff. These are part of the same story
This is not intended. What issue do you currently see on v4? |
thanks for the feedback, I also doubt myself if I should do it. I will revert the sponsors section.
I think you misunderstand my point or I misunderstand Olivier's point. I am all in for improving performance on the homepage. However, Olivier pointed out here that the approach "is not a method that developers can't use at scale to solve slow landing page performance issues". That's why I responded that the PR try to improve the performance not building method for other developers. |
I have no idea what Olivier is talking about. We should have a slow landing page because that's what user should expect? This is news to me. A major aspect of the new styling solution was performance. |
merged to unblock refinement. If @oliviertassinari does not agree with this approach, we can discuss once you are back. |
@siriwatknp I have noticed a minor issue with the sizes, there is one last layout shift. I'm sending a PR to address it: #27930. It looks great. To clarify #27838 (comment). We have acknowledged that the lazy mount approach costs developers' time, a scarce resource:
If we assume that the lazy mount approach is not something that most developers have the time to leverage, would it mean that:
? Parts of the pages are always rendered, so I think that we will still get the confirmation feedback loop if we improve the performance of the styles/system. All good. Regarding the performance of the styles in v5 vs. v4. The best summary I'm aware of is https://stackoverflow.com/questions/68383046/is-there-a-performance-difference-between-the-sx-prop-and-the-makestyles-functio, the change of performance depends on the API. We did a benchmark at the component level at some point: #22342 (comment). v5 Slider was about 25% slower than v4 but we moved forward anyway, judging that the value of dynamic styles was well worth the slowdown. It could be interesting to update the benchmark, to get a more accurate view now, vs. before. |
Preview: https://deploy-preview-27838--material-ui.netlify.app/branding/home/
Changes
remove font blocking resource(will be in another PR before switching to mui.com)react-intersection-observer
PlusJakartaSans
(40kB -> 7kB) and improve UX with preload (no more font swap!)Summary
Desktop
Full test (Before, After)
Mobile
Full test (Before, After)
Comparison between
/branding/home/
&/
Before improvement
index is faster than rebranding
Full test link
After improvement
Both routes are faster than
before
because font blocking resource is changed to async. Rebranding is a bit faster than index 🎉Full test link