frontend speed improvements - cuts landing page load time by 55% and DOMready time by 35% #4826
Conversation
owocki
requested review from
danlipert,
octavioamu and
thelostone-mc
as code owners
|
updated from #4791 |
| @@ -100,6 +100,13 @@ <h1 class="text-center title">{% trans "Submit Work" %}</h1> | |||
| {% include 'shared/footer.html' %} | |||
| {% include 'shared/messages.html' %} | |||
| </body> | |||
|
|
|||
| <!-- jQuery --> | |||
There was a problem hiding this comment.
this is already loaded in footer_scripts.js
| @@ -201,6 +201,13 @@ <h5>{% trans 'Payout Preview' %}</h5> | |||
| {% include 'shared/footer.html' %} | |||
| {% include 'shared/messages.html' %} | |||
| </body> | |||
|
|
|||
| <!-- jQuery --> | |||
There was a problem hiding this comment.
this is already loaded in footer_scripts.js
There was a problem hiding this comment.
got it; ill factor it back into footer_scripts
There was a problem hiding this comment.
cool I will wait until you finish to review it, ping me when ready
There was a problem hiding this comment.
just updated 1eb3739
| @@ -109,8 +109,15 @@ <h3>{% trans "No results found." %}</h3> | |||
| {% include 'shared/footer_scripts.html' %} | |||
| {% include 'shared/footer.html' %} | |||
| {% include 'shared/messages.html' %} | |||
|
|
|||
| {% compress js %} | |||
| <script src="{% static "v2/js/lib/popper.min.js" %}"></script> | |||
There was a problem hiding this comment.
this is already loaded in footer_scripts.js
| @@ -251,7 +251,18 @@ <h6 class="font-weight-bold mb-3">Invite User to Bounty</h6> | |||
| {% include 'shared/footer.html' %} | |||
| {% include 'shared/messages.html' %} | |||
|
|
|||
| {% compress js %} | |||
| <script src="{% static "v2/js/lib/popper.min.js" %}"></script> | |||
There was a problem hiding this comment.
this is already loaded in footer_scripts.js
| <script> | ||
| let bootstrapTooltip = $.fn.tooltip.noConflict() | ||
| $.fn.runTooltip = bootstrapTooltip; |
There was a problem hiding this comment.
also declared in footer_scripts
which dependancies are you worried about? crawling the site now, not seeing any conflicts |
|
i think i got everyones feedback so far, but lmk if not |
app/kudos/templates/kudos_about.html
Outdated
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
could we remove the extra lines?
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
could we remove extra lines?
|
|
||
|
|
||
|
|
||
|
|
requirements/base.txt
Outdated
| @@ -82,3 +82,8 @@ sentry-sdk | |||
| websocket-client | |||
| bleach | |||
| python-magic | |||
| rcssmin --install-option="--without-c-extensions" | |||
| rjsmin --install-option="--without-c-extensions" | |||
| django-compressor --upgrade | |||
There was a problem hiding this comment.
Do we need the --upgrade option?
An auto-upgrade might mess things up if there ever is a breaking upgrade for compressor
Also travis fails
ERROR: Invalid requirement: django-compressor --upgrade
pip: error: no such option: --upgrade
The command "pip install -r requirements/test.txt" exited with 1.
There was a problem hiding this comment.
Agree - having auto-upgrade is a bit scary.
|
@owocki A couple of other concerns
This would make debugging a lil tricker but the performance boost outweighs it 🙌 aka all library css files would be it's own bundle and all custom would be it's own bundle I'm assuming this can be with something as simple as |
|
^ @owocki on the off chance you didn't see this |
you can toggle this in your environment by setting or were you worried about prod debugging?
whats the difference between 'library' css files and custom? is the former dependancies like jquery / bootstrap, and the the latter is our own css files? |
|
@owocki And yes former == dependencies |
|
@owocki @thelostone-mc Funny thing. You are worried about debugging on production and I will be happy to see it and I will be glad if such production debugging appears. HACKING ❤️ But you're right, you really have to be careful about it. Sometimes very sensitive information is found in such debugging. |
got it. whats your existing prod debugging workflow? i assume its sentry > js errors > repro locally > submit patch? how often do we go through this workflow? |
|
about production debugging the answer is "source maps" but not sure if that django tool have it |
|
sourcemaps seems super powerful! seems like that solves the production debugging issue, no? i will update the css/js builds to make them into dependancy/gitcoin packages and then re-submit the PR. does that sounds good? seems like thats where the consensus is going here.. |
requirements/base.txt
Outdated
| @@ -82,3 +82,8 @@ sentry-sdk | |||
| websocket-client | |||
| bleach | |||
| python-magic | |||
| rcssmin --install-option="--without-c-extensions" | |||
| rjsmin --install-option="--without-c-extensions" | |||
| django-compressor --upgrade | |||
app/dashboard/models.py
Outdated
| @@ -301,7 +301,7 @@ class Bounty(SuperModel): | |||
| canceled_bounty_reason = models.TextField(default='', blank=True, verbose_name=_('Cancelation reason')) | |||
| project_type = models.CharField(max_length=50, choices=PROJECT_TYPES, default='traditional', db_index=True) | |||
| permission_type = models.CharField(max_length=50, choices=PERMISSION_TYPES, default='permissionless', db_index=True) | |||
| bounty_categories = ArrayField(models.CharField(max_length=50, choices=BOUNTY_CATEGORIES), default=list) | |||
| bounty_categories = ArrayField(models.CharField(max_length=50, choices=BOUNTY_CATEGORIES), default=list, blank=True) | |||
There was a problem hiding this comment.
hmmm i can't seem to generate one
bash-4.4# ./manage.py makemigrations
No changes detected
i thikn ebecause master has this change (and a subssequent migration) also
app/kudos/templates/kudos_about.html
Outdated
|
|
||
|
|
||
|
|
||
|
|
|
|
||
|
|
||
|
|
||
|
|
|
|
||
|
|
||
|
|
||
|
|
| <script src="https://cdn.jsdelivr.net/npm/vue"></script> | ||
| {% elif user.is_authenticated %} | ||
| <script src="https://cdn.jsdelivr.net/npm/vue/dist/vue.js"></script> | ||
| <script src="https://cdn.jsdelivr.net/npm/vue"></script> |
There was a problem hiding this comment.
i dont think well be running ./manage.py compress on our local, so youll still be able to debug on your local with the local one
| <script src="{% static "v2/js/lib/popper.min.js" %}"></script> | ||
| <script src="{% static "v2/js/lib/bootstrap.min.js" %}" crossorigin="anonymous"></script> | ||
| <script src="{% static "jsi18n/en/djangojs.js" %}"></script> | ||
| <script src="{% static "v2/js/lib/vue.js" %}"></script> |
There was a problem hiding this comment.
i favor bringing vue inside of the compress; but @octavioamu let me know if u disagree
|
ok this is ready for re-review |
You might want to revert that later for windows compatibility, or add a flag for each OS
|
@owocki I can't get the django-compressor library to install in my image, here's the output: |
|
@danlipert checkout the requirements/base file ( https://github.com/gitcoinco/web/pull/4826/files#diff-7fe11226cff646f5d9f35faa76217059 ) if you install rcssmin with the flags in there itll work |
Kevin/site-perf-3 Improve loading speed with django compressor
Codecov Report
@@ Coverage Diff @@
## master #4826 +/- ##
==========================================
- Coverage 30.86% 30.86% -0.01%
==========================================
Files 219 219
Lines 17656 17623 -33
Branches 2435 2427 -8
==========================================
- Hits 5450 5439 -11
+ Misses 11981 11959 -22
Partials 225 225
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #4826 +/- ##
==========================================
- Coverage 30.86% 30.86% -0.01%
==========================================
Files 219 219
Lines 17656 17623 -33
Branches 2435 2427 -8
==========================================
- Hits 5450 5439 -11
+ Misses 11981 11959 -22
Partials 225 225
Continue to review full report at Codecov.
|
|
@MajorTomSec got your PR in but that breaks a lot of this when I pull this in locally ! Could you test out all your changes and raise a new PR to the branch ensuring none of your changes break the existing behavior ? If there are stuff which you can't test / figure out why it's breaking -> it might make more sense to not compress them at all! cc @owocki I can't get this in unless the we are sure the PR doesn't break existing functionality |
I had no issue testing it locally (even though I could very well have missed things !), at least from what I could see. Can you point out issues you're having so I can try to reproduce them ? |
| {% include 'shared/sentry.html' %} | ||
| <script src="{% static "v2/js/lib/popper.min.js" %}"></script> | ||
| <script src="{% static "v2/js/lib/bootstrap.min.js" %}" crossorigin="anonymous"></script> | ||
| <script> | ||
| const bootstrapModal = $.fn.modal.noConflict() |
There was a problem hiding this comment.
this is going to give $.fn.modal is undefined as bootstrap is loaded after this declaration.
| <script src="{% static "v2/js/lib/jquery-ui.js" %}"></script> | ||
| <script src="{% static "v2/js/lib/tooltip.js" %}"></script> | ||
| {% endif %} | ||
| <script src="{% static "v2/js/lib/jquery-ui.js" %}"></script> |
There was a problem hiding this comment.
this will break a lot of places using modals and popovers from bootstrap, that the main reason of slim, also as we are trying to totally remove this from the product
|
Closing this out cause @octavioamu and @androolloyd were talking about redoing this with compress and @octavioamu already has a open PR we can use as a starting point |
Description
up until now, my efforts on site speed have been mostly oriented around HTML generation time. nwo that we've gotten the HTML gen time down, i'm turning my efforts towards focusing more on front-end optimizations.
this is a PR that takes the landing page load time on gitcoin from
to
on the intial page paint, thats:
on a cold cache.
on a warmed cache itll take us from
how
further TODOs
Testing
tested landing page locally