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

refactor: Merge header & sponsor SVGs into sprite sheets #1032

Merged
merged 6 commits into from
Dec 7, 2023

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Dec 3, 2023

Few less requests (9 -> 3), bit smaller transfer size (68.29kb -> 61.76kb), no change is usability.

I did take a look at merging the other SVGs on the home page, but we actually benefit from the lazy loading there a fair bit -- on most screens the first icon will immediately loaded, which would drag in the entire sprite sheet. This is a ~6.5kb difference on page load, so not worth it.

width: 1.6rem;
svg {
width: 1.5rem;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit of an odd one -- 1.6 for some reason causes some clipping/size calculation issue. Not terribly sure why

Comment on lines +20 to +26
{
link: 'https://scams.info',
title: 'scams.info',
source: ScamsInfo,
width: '240',
height: '240'
},
Copy link
Member Author

Choose a reason for hiding this comment

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

This site looks a tad sketchy btw, gambling info site but the authors appear to be stock images and I cannot detect Preact in use (doesn't seem like the sort of site/business to be using Preact in other places either). Looks like Andre added them in #955, they didn't implement this themselves.

Coincidentally, they're the only ones with a non-SVG logo, couldn't find any SVG on their site or otherwise.

@@ -1 +0,0 @@
<svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 63 63"><linearGradient id="a" x1="34.909" x2="7.632" y1="61.029" y2="13.785" gradientUnits="userSpaceOnUse"><stop offset="0" stop-color="#1e8e3e"/><stop offset="1" stop-color="#34a853"/></linearGradient><linearGradient id="b" x1="26.904" x2="54.181" y1="63.079" y2="15.835" gradientUnits="userSpaceOnUse"><stop offset="0" stop-color="#fcc934"/><stop offset="1" stop-color="#fbbc04"/></linearGradient><linearGradient id="c" x1="4.221" x2="58.775" y1="19.688" y2="19.688" gradientUnits="userSpaceOnUse"><stop offset="0" stop-color="#d93025"/><stop offset="1" stop-color="#ea4335"/></linearGradient><path fill="#fff" d="M31.499 47.247c8.698 0 15.75-7.052 15.75-15.75s-7.052-15.75-15.75-15.75-15.75 7.051-15.75 15.75 7.051 15.75 15.75 15.75z"/><path fill="url(#a)" d="M17.86 39.375 4.22 15.754a31.492 31.492 0 0 0 27.28 47.242l13.638-23.62v-.005a15.746 15.746 0 0 1-27.28.004z"/><path fill="url(#b)" d="M45.138 39.374 31.5 62.995A31.491 31.491 0 0 0 58.773 15.75H31.498l-.003.002a15.747 15.747 0 0 1 13.643 23.623z"/><path fill="#1a73e8" d="M31.499 43.969c6.886 0 12.469-5.583 12.469-12.469s-5.583-12.469-12.469-12.469S19.03 24.614 19.03 31.5 24.613 43.969 31.5 43.969z"/><path fill="url(#c)" d="M31.5 15.75h27.274a31.493 31.493 0 0 0-54.553.004L17.86 39.375l.004.002A15.747 15.747 0 0 1 31.499 15.75z"/></svg>
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a slight issue here in that external gradients are a spec dead zone and there are open bugs in Chrome & FireFox (likely Safari too) that are 12+ years old with absolutely no movement. The Chrome and Songsterr icons use these gradients, but they're incredibly subtle and you'll only notice if you compare them side-by-side (or switch between the preview & deployed site w/ hover styles set in devtools).

I therefore think it's fine to strip them out, but wanted to bring it up.

width="34"
height="33"
/>
<svg aria-hidden viewBox="0 0 24 24">
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we're meant to use aria-hidden here, as it's just content of the link and not relevant to screen readers?

height: 4.5rem;
img,
svg {
height: 3rem;
Copy link
Member Author

Choose a reason for hiding this comment

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

We had inline styles setting height: 3rem, so moved here.

@rschristian rschristian marked this pull request as ready for review December 3, 2023 07:49
@rschristian
Copy link
Member Author

There's potentially an opportunity to add the magnifying glass symbol from the search bar in as well, but a few days of poking at it and the styling was still kicking my butt so I give in. Benefits would be arguable anyhow.

@rschristian rschristian merged commit 125e03a into master Dec 7, 2023
4 checks passed
@rschristian rschristian deleted the refactor/merge-svgs branch December 7, 2023 05:08
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.

2 participants