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

Drop SVG sprite for font on docs homepage #824

Merged
merged 8 commits into from Dec 27, 2022
Merged

Drop SVG sprite for font on docs homepage #824

merged 8 commits into from Dec 27, 2022

Conversation

mdo
Copy link
Member

@mdo mdo commented Mar 24, 2021

Drop usage of large SVG sprite on homepage and in copy buttons, replacing them with the icon fonts. Goal is to improve performance and page load since the sprite is so large.

TODO:

  • Fix line-height discrepancy
  • Redirect font page to Sprite page
  • Fix Sprite page SVG 404
  • Fix clipboard icon TypeError on click

Preview:

@mdo mdo added the docs Improvements or additions to documentation label Mar 24, 2021
@mdo mdo requested a review from XhmikosR March 24, 2021 20:26
@XhmikosR
Copy link
Member

Not sure if this improves things a lot. I mean, it certainly reduces the total bytes downloaded, but I believe the culprit is the HTML size/number of DOM elements, which is still the same. That being said, from a quick look, it shouldn't hurt.

Avoid an excessive DOM size | 7,236 elements

Give me a few days to test this out :)

@mdo
Copy link
Member Author

mdo commented Mar 24, 2021

No rush! Appreciate the eyes on it. I'd also be in favor of keeping SVGs and showing things differently on the homepage. Maybe show latest release or specific categories? Or show them all by categories? Open to ideas.

@XhmikosR XhmikosR marked this pull request as ready for review March 29, 2021 06:05
@XhmikosR
Copy link
Member

@mdo LGTM, let's land it. My only suggestion would be to maybe add a new sprite page later so that we have a visual test for the sprite?

@mdo mdo added this to In progress in v1.5.0 via automation Mar 30, 2021
@XhmikosR
Copy link
Member

@mdo I say we move with #706 and then I'll adapt this PR to include a sprite page (open to suggestions for the URL)

@mdo mdo removed this from In progress in v1.5.0 May 8, 2021
@XhmikosR XhmikosR mentioned this pull request Nov 2, 2021
5 tasks
@XhmikosR XhmikosR marked this pull request as draft November 2, 2021 07:06
@XhmikosR XhmikosR force-pushed the fonts-on-homepage branch 4 times, most recently from ba63b96 to 5392a22 Compare November 25, 2022 17:29
@XhmikosR
Copy link
Member

@mdo I pushed some patches, now it looks better I think. If it LGTY too, let's land this :)

@XhmikosR XhmikosR marked this pull request as ready for review November 26, 2022 06:40
@XhmikosR XhmikosR force-pushed the fonts-on-homepage branch 2 times, most recently from 03b76c5 to f30f0be Compare November 26, 2022 06:43
@XhmikosR
Copy link
Member

XhmikosR commented Nov 26, 2022

We save ~2000 (-16%) DOM elements and ~1.3MB (-45%) uncompressed / ~200KB (-35%) brotli transfer size for homepage.

Still far from perfect, but until we figure out a way to use pagination or something else, this should improve things noticeably, especially on slow devices/browsers.

@XhmikosR XhmikosR force-pushed the fonts-on-homepage branch 2 times, most recently from da20f29 to f276d9a Compare November 27, 2022 15:56
@XhmikosR
Copy link
Member

@patrickhlauke do you think we are missing something here a11y-related?

@mdo mdo merged commit 9b8db82 into main Dec 27, 2022
@mdo mdo deleted the fonts-on-homepage branch December 27, 2022 01:34
@patrickhlauke
Copy link
Member

generally, most folks are encouraged to move away from icon fonts in favour of SVGs (to make sure that even if a user, for their specific needs, explicitly changes font to make it more readable for themselves, icons remain as they are), but beyond that, sure...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation no-squash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants