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

Compressed share icons #36955

Merged
merged 7 commits into from
Nov 18, 2021
Merged

Conversation

mszylkowski
Copy link
Contributor

@mszylkowski mszylkowski commented Nov 17, 2021

Improved bundle size with 2 optimizations:

  • Google+ is an obsolete share icon but the assets were still in the bundle (svgs and localization), allowing us to remove 0.3kB from the bundle. Original I2R.
  • All SVGs were not properly compressed (using SVGOMG to precision=2 results in no noticeable differences in quality but compress the icons with 0.7kB of gains in the bundle size of amp-story and amp-social-share). The svgs were also URL encoded, which is unnecessary (AFAIK) but adds more characters to the svg asset.

Icons still look the same:

image

image

@mszylkowski mszylkowski self-assigned this Nov 17, 2021
@mszylkowski mszylkowski marked this pull request as ready for review November 17, 2021 15:02
@amp-owners-bot amp-owners-bot bot requested a review from rsimha November 17, 2021 15:02
@amp-owners-bot
Copy link

amp-owners-bot bot commented Nov 17, 2021

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/_locales/af.json
extensions/amp-story/1.0/_locales/am.json
extensions/amp-story/1.0/_locales/ar.json
extensions/amp-story/1.0/_locales/bg.json
extensions/amp-story/1.0/_locales/bn.json
extensions/amp-story/1.0/_locales/bs.json
extensions/amp-story/1.0/_locales/ca.json
extensions/amp-story/1.0/_locales/cs.json
extensions/amp-story/1.0/_locales/da.json
extensions/amp-story/1.0/_locales/de.json
extensions/amp-story/1.0/_locales/el.json
extensions/amp-story/1.0/_locales/en-GB.json
+61 more

@gmajoulet
Copy link
Contributor

What's the impact on publishers that configured Google+ sharing? If it was already deprecated or disabled, can you link to the I2D/R or the PR dropping support for it?

@mszylkowski
Copy link
Contributor Author

mszylkowski commented Nov 17, 2021

When you add "gplus" to the configuration, that option is disabled anyways. Currently it hides the icon but leaves the gap, which is very weird. I propose we directly don't show it, the original PR hints that the current behavior is not great, but we never got to fix it.

Jon: Testing it out real quick by just display: noneing the element, it looks like this leaves a gap in the share sheet where there icon was supposed to be. We structure it as a list, with the list items having the size; we can refactor to not do that.

Current:
image

New:
image

@gmajoulet
Copy link
Contributor

@kristoferbaxter for SVG approval - for context this is saving .9kB, or 1.2% of the overall story bundle

}

.amp-social-share-line {
background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 511.99"><defs><style>.cls-1{fill:#fff;}</style></defs><title>名称未設定-1</title><g id="レイヤー_1" data-name="レイヤー 1"><path class="cls-1" d="M443.2,233.29c0-84.14-84.35-152.6-188-152.6s-188,68.46-188,152.6c0,75.43,66.9,138.61,157.26,150.55,6.13,1.32,14.46,4,16.57,9.27,1.89,4.76,1.24,12.2.61,17,0,0-2.21,13.26-2.69,16.09-.82,4.75-3.78,18.6,16.29,10.14S363.45,372.58,402.9,327.18h0C430.15,297.29,443.2,267,443.2,233.29ZM188.82,278.21a3.67,3.67,0,0,1-3.66,3.67H132.47a3.6,3.6,0,0,1-2.53-1l-.06-.05,0-.05a3.65,3.65,0,0,1-1-2.53h0V196.29a3.66,3.66,0,0,1,3.66-3.66h13.19a3.66,3.66,0,0,1,3.66,3.66v65.07h35.84a3.66,3.66,0,0,1,3.66,3.66Zm31.8,0a3.65,3.65,0,0,1-3.66,3.65h-13.2a3.65,3.65,0,0,1-3.66-3.65V196.29a3.66,3.66,0,0,1,3.66-3.66H217a3.66,3.66,0,0,1,3.66,3.66Zm90.78,0a3.65,3.65,0,0,1-3.66,3.65H294.55a3.67,3.67,0,0,1-.94-.12h-.05l-.25-.08-.11,0-.18-.08-.17-.08-.11-.06-.22-.14,0,0a3.45,3.45,0,0,1-.93-.9L254,229.56v48.66a3.66,3.66,0,0,1-3.67,3.65H237.1a3.65,3.65,0,0,1-3.66-3.65V196.29a3.66,3.66,0,0,1,3.66-3.66h13.51l.19,0,.16,0,.21.05.13,0,.21.07.12,0a1.31,1.31,0,0,1,.21.08l.12.06.19.11a.41.41,0,0,1,.11.07l.19.13.1.07.19.16.07.07a2.28,2.28,0,0,1,.22.22l0,0a3.58,3.58,0,0,1,.28.37L290.89,245V196.29a3.66,3.66,0,0,1,3.66-3.66h13.19a3.66,3.66,0,0,1,3.66,3.66Zm72.83-68.74a3.66,3.66,0,0,1-3.65,3.67H344.74V227h35.84a3.66,3.66,0,0,1,3.65,3.67v13.19a3.65,3.65,0,0,1-3.65,3.66H344.74v13.85h35.84a3.65,3.65,0,0,1,3.65,3.66v13.19a3.66,3.66,0,0,1-3.65,3.67h-52.7a3.66,3.66,0,0,1-2.53-1l-.05-.05a.12.12,0,0,1-.05-.05,3.65,3.65,0,0,1-1-2.53h0V196.3h0a3.6,3.6,0,0,1,1-2.52l.06-.07s0,0,0,0a3.63,3.63,0,0,1,2.54-1h52.7a3.66,3.66,0,0,1,3.65,3.67Z"/></g></svg>');
background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512"><path fill="#fff" d="M443 233c0-84-84-152-188-152S67 149 67 233c0 76 67 139 157 151 7 1 15 4 17 9s1 12 1 17l-3 16c-1 5-4 19 16 10s108-63 148-109c27-30 40-60 40-94Zm-254 45a4 4 0 0 1-4 4h-53a4 4 0 0 1-2-1 4 4 0 0 1-1-3v-82a4 4 0 0 1 4-3h13a4 4 0 0 1 3 3v65h36a4 4 0 0 1 4 4Zm32 0a4 4 0 0 1-4 4h-13a4 4 0 0 1-4-4v-82a4 4 0 0 1 4-3h13a4 4 0 0 1 4 3Zm90 0a4 4 0 0 1-3 4h-13a4 4 0 0 1-1 0h-1v-1a3 3 0 0 1-1-1l-38-50v48a4 4 0 0 1-4 4h-13a4 4 0 0 1-4-4v-82a4 4 0 0 1 4-3h16v1l38 51v-49a4 4 0 0 1 4-3h13a4 4 0 0 1 3 3Zm73-69a4 4 0 0 1-3 4h-36v14h36a4 4 0 0 1 3 4v13a4 4 0 0 1-3 4h-36v13h36a4 4 0 0 1 3 4v13a4 4 0 0 1-3 4h-53a4 4 0 0 1-3-1 4 4 0 0 1-1-3v-82a4 4 0 0 1 1-2 4 4 0 0 1 3-1h53a4 4 0 0 1 3 3Z" data-name="レイヤー 1"/></svg>');
Copy link
Contributor

Choose a reason for hiding this comment

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

These should all have ;charset=utf-8 directly after svg+xml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, especially since they have the title in non-ascii on the Line icon.
As a counter argument, I've seen most icons have only ASCII chars, which is the default encoding, so we could remove the utf8 for those cases since the default value is US-ASCII. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's an issue with some browsers when the chraset is missing, even if the contents are entirely ASCII. Essentially treat everything on the web as UTF8, even when it's not.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mszylkowski we should fix this asap the bug is in beta

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be fixed by #36987 and #37017.

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.

5 participants