-
Notifications
You must be signed in to change notification settings - Fork 2
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
Reduce Layout shifts, Inline Styling - Set fixed height/width where possible #2075
Conversation
db1cc07
to
36be99c
Compare
36be99c
to
cfca481
Compare
Codecov Report
@@ Coverage Diff @@
## master #2075 +/- ##
==========================================
- Coverage 88.00% 87.99% -0.02%
==========================================
Files 328 328
Lines 14767 14767
Branches 1031 1031
==========================================
- Hits 12996 12994 -2
- Misses 1532 1534 +2
Partials 239 239
Continue to review full report at Codecov.
|
cfca481
to
97805cf
Compare
97805cf
to
aaf88f9
Compare
) or CoursePage.objects.filter( | ||
featured=True, course__live=True | ||
) | ||
featured_product = program_page_qset.filter( |
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 change just reuses the queryset
that we created above for Program and Course Pages, Plus we were not checking for .live()
while obtaining the Featured Product.
I'd recommend getting rid of the other PR that this is rebased on, and just keeping this PR. It's not necessarily in our interests to break these performance PRs up by CMS page. The performance improvements in general are strongly linked to each other, and there are limited examples of improvements that are only relevant to these specific pages. |
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.
@arslanashraf7 Could you split this into 2 pull requests? (1) should contain the lazy div loading code. (2) should contain everything else. I'm concerned that that lazy div loading opens us up to weird/buggy behavior for questionable performance gains. We can set that lazy div loading PR to "On Hold", and we can review/merge it if it seems like our other less-risky changes are not having the desired effect
@gsidebo I Totally agree with you, I was also very much inclined towards the division of this PR. I'll divide this PR into two and also check if there are more elements in general where we could improve layout shifts. As for division, I'll keep all the Non |
5b70053
to
5c18d79
Compare
5c18d79
to
2345671
Compare
2345671
to
69397bc
Compare
69397bc
to
10436f2
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.
Pages load as expected. Ship it! 👍
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.
(Sorry, let's try that again...) Pages load as expected. Ship it! 👍
Pre-Flight checklist
What are the relevant tickets?
Related to #2065
What's this PR do?
Attempts to optimize the Catalog & Product Pages through suggestions provided by Lighthouse
How should this be manually tested?
Open any of the Catalog/Product and check the statistics on
LightHouse
, Make sure that the page loads in a better amount of time than before. Make sure that we caught maximum possible suggestions through Lighthouse regarding images layout shifts, setting height/width of images.