-
-
Notifications
You must be signed in to change notification settings - Fork 775
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
Kevin/site perf2 #4800
Kevin/site perf2 #4800
Conversation
you got the PR to open! it was so big github's UI choked on the PR creation page for me :) |
hah! yeah I had to try it a few times |
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.
LGTM ! Had a minor doubt though ( I've left a comment for those )
@danlipert / @octavioamu you guys wanna sit and resolve all these before next deploy? |
we really only need the image changes from this PR.. not the html changes (which are managed on #4826 already ) |
@danlipert / @octavioamu So spent an annoying 1 hour resolving conflicts and ensuring only img compression happen @owocki I've removed the compress + other html changes in this PR This PR now has
|
@@ -17,7 +17,7 @@ | |||
{% endcomment %} | |||
{% load i18n static %} | |||
<footer class="container-fluid no-gutters footer"> | |||
<div class="container footer__main"> | |||
<div class="container footer__main" data-src="{% static "v2/images/footer.png" %}"> |
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.
how this work?
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.
jquery-unveil.js loads it iff the user scrolls down to the footer (not upon pageload)
Closing this out and opening this up against master |
Description
Compressed images for site performance
Refers/Fixes
#4791