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

Kevin/site-perf-3 Improve loading speed with django compressor #5106

Merged
merged 6 commits into from
Sep 4, 2019
Merged

Kevin/site-perf-3 Improve loading speed with django compressor #5106

merged 6 commits into from
Sep 4, 2019

Conversation

MajorTomSec
Copy link
Contributor

Description

This PR adds many compress tags for both JS and CSS static files.
Using python3 manage.py compress --follow-links --force, we create a cache used for serving the aggregated minimized static files, thus improving the loading speed of the platform for the end user.

This PR also fixes some typos and issues from previous commits, and temporarily fixes a bug where the building process would fail on linux, due to some options passed to pip. This needs to be fixed in a better way in order to improve compatibility across different OS.

This PR needs to be reviewed, as a lot of changes have been made. I can confirm the loading time is significantly improved through these changes, but this needs more testing and reviewing to confirm this doesn't break anything.

python3 manage.py compress --follow-links --force successfully passes all templates, except the following:
Compressing... Error parsing template /code/app/retail/templates/emails/grants/transaction_summary.html: code/app/retail/templates/emails/emails/template.html Error parsing template /code/app/retail/templates/emails/grants/update_notification.html: code/app/retail/templates/emails/emails/template.html done Compressed 158 block(s) from 256 template(s) for 0 context(s).

I have not been able to fix this issue. It was not introduced in this PR and already happened when I started working on @owocki 's base PR.

Refers/Fixes

This is a PR aiming to fix issue #5091
This is based on @owocki 's work from PR #4826

You might want to revert that later for windows compatibility, or add a flag for each OS
<script src="{% static "v2/js/jobs.js" %}"></script>
<script src="{% static "v2/js/avatar_builder.js" %}" defer></script>
{% endcompress %}
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to move any js files that are not compressable (because they are hosted elsewhere, or they are dynamically generated like tokens_dynamic) to below the compress block and then only have ONE compress block?

Copy link
Contributor

Choose a reason for hiding this comment

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

this applies in more than just this file ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'm working on that !

@owocki
Copy link
Contributor

owocki commented Sep 2, 2019

this looks really good.

just had one question, then can merge to #4826

@thelostone-mc
Copy link
Member

thelostone-mc commented Sep 3, 2019

@MajorTomSec I'm gonna try and get @owocki PR in tom !
If you could get your PR approved before that, sweet !
If not you'd have to close this PR and open it against master

Also, i believe we can go ahead without compressing any of the email stuff ! That's fine

Neat job on commit 561f1af

PS: conflict resolution needed

@MajorTomSec
Copy link
Contributor Author

Last commit fixed conflict @thelostone-mc, thanks for pointing it out.
I just pushed the modifications @owocki requested, anyway !
Let me know if anything else needs to be changed !

@owocki
Copy link
Contributor

owocki commented Sep 3, 2019

PR looks good to me; as long as python3 manage.py compress --follow-links --force runs we good

@thelostone-mc thelostone-mc merged commit fbd7963 into gitcoinco:kevin/site-perf-3 Sep 4, 2019
@thelostone-mc
Copy link
Member

@MajorTomSec mind checking out the comment here :
#4826 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants