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

Add icon sprite via local storage #6243

Merged
merged 8 commits into from Sep 24, 2020

Conversation

allcaps
Copy link
Member

@allcaps allcaps commented Jul 19, 2020

This PR stores the sprite in local storage and injects it into the page body.

A previous attempt has an external sprite with IE11 polyfill. #6231
Unfortunately, it leads to flashing icons in Firefox. :/

This PR:

  • Inspired by https://osvaldas.info/caching-svg-sprite-in-localstorage
  • Uses local storage
  • Injects the sprite into the body
  • The filename contains a hash to invalidate local storage.
  • The hash is based on the sprite contents and secret key (inspired by versioned_static).
  • No polyfill at all 🎉
  • No flash on load 🎉

Checks:

  • Do the tests still pass? ✅
  • Does the code comply with the style guide? ✅
  • For Python changes: Have you added tests to cover the new/fixed behaviour? ✅
  • For front-end changes: Did you test on all of Wagtail’s supported browsers? ✅
    • ✅ IE 11
    • ✅ Edge 83
    • ✅ Chrome 84.0.4147.89
    • ✅ Firefox 78.0.2
    • ✅ Firefox 68.10.0esr
    • ✅ Safari 13.1.2
    • ✅ Safari 13.1 iPad
  • For new features: Has the documentation been updated accordingly? N/A

@squash-labs
Copy link

squash-labs bot commented Jul 19, 2020

Manage this branch in Squash

Test this branch here: https://allcapsicon-sprite-local-stora-pzwaj.squash.io

@allcaps allcaps force-pushed the icon-sprite-local-storage branch 2 times, most recently from f7f1e58 to 2972b8d Compare July 26, 2020 20:28
@allcaps allcaps marked this pull request as ready for review July 26, 2020 21:01
@allcaps allcaps requested a review from Scotchester July 27, 2020 08:54
Copy link
Contributor

@Scotchester Scotchester left a comment

Choose a reason for hiding this comment

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

Coen!! This is brilliant! I'm so glad you found this technique. It's working great for me (tested in Firefox, Chrome, Safari, and IE11). I would like @thibaudcolas to review the JS specifically, but I do have one small question about it.

wagtail/admin/static_src/wagtailadmin/js/core.js Outdated Show resolved Hide resolved
wagtail/admin/static_src/wagtailadmin/js/core.js Outdated Show resolved Hide resolved
wagtail/admin/static_src/wagtailadmin/js/core.js Outdated Show resolved Hide resolved
wagtail/admin/static_src/wagtailadmin/js/core.js Outdated Show resolved Hide resolved
wagtail/admin/static_src/wagtailadmin/js/core.js Outdated Show resolved Hide resolved
wagtail/admin/static_src/wagtailadmin/js/core.js Outdated Show resolved Hide resolved
wagtail/admin/static_src/wagtailadmin/js/core.js Outdated Show resolved Hide resolved
wagtail/admin/static_src/wagtailadmin/js/core.js Outdated Show resolved Hide resolved
wagtail/admin/static_src/wagtailadmin/js/core.js Outdated Show resolved Hide resolved
@thibaudcolas thibaudcolas added this to the 2.11 milestone Aug 1, 2020
@allcaps
Copy link
Member Author

allcaps commented Aug 4, 2020

All feedback is applied.

I moved the script into the page body. This way it is executed on load. All icons are directly available for paint.
This removes the annoying flash in Firefox! 🎉

What do we do with the catched error? Not displaying icons is an obvious fail. I propose we don't handle the error at all.

@thibaudcolas
Copy link
Member

I’ll try and review this over the next two weeks. @allcaps is there anything in particular you know might not be ready to go?

@allcaps
Copy link
Member Author

allcaps commented Sep 11, 2020

Ready!

@allcaps
Copy link
Member Author

allcaps commented Sep 22, 2020

I rebased and simplified the code a bit. I dropped the template tag. Now the view itself is cached. This is less code and easier to understand.

@allcaps
Copy link
Member Author

allcaps commented Sep 22, 2020

Ready again :D

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

@allcaps this is looking really good! The icons are missing for a small amount of time on the first page load, but this is similar to an icon font’s performance, and for subsequent page loads the SVG icons seem to load faster than the icon fonts’.

I’ve suggested a few more changes but they are very minor.

For future reference, I tried to improve the first load performance with <link rel="preload" href="{% url 'wagtailadmin_sprite' %}" as="fetch"> towards the top of skeleton.html. This works as expected, however it has the drawback of always being there, even after the first load, so feels very wasteful for a one-off performance gain. I don’t think this is worthwhile.

wagtail/admin/urls/__init__.py Outdated Show resolved Hide resolved
wagtail/admin/templates/wagtailadmin/skeleton.html Outdated Show resolved Hide resolved
wagtail/admin/templates/wagtailadmin/skeleton.html Outdated Show resolved Hide resolved
Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Wooo!

@thibaudcolas thibaudcolas merged commit 0338cc3 into wagtail:master Sep 24, 2020
@thibaudcolas
Copy link
Member

Rebased, merged, and ready for release in 2.11! Thank you @allcaps, it looks like a lot of thought went into this, and it’s working perfectly as far as I can tell. I haven’t done any thorough benchmarking but this looks like it will improve first page load performance to be as good as the best external sprites implementation, while subsequent page loads will be as fast as the best inline sprites implementations – all the while preserving compatibility with IE11. Cool stuff :)

@allcaps allcaps deleted the icon-sprite-local-storage branch September 25, 2020 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants