search

Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.59k stars 798 forks source link

Sharing / Theme Tools: update social icons to use single SVGs instead of icon font or SVG sprite #28861

Open jeherve opened 1 year ago

jeherve commented 1 year ago

The Jetpack plugin currently ships with 3 versions of elements of https://github.com/Automattic/social-logos :

While this works today, it has a few inconvenients:

  1. This complexity makes adding a new logo a multi-step process: add the logo in the source repo, release a new version of the package, start using the package in the Jetpack plugin, update the SVG sprite in the Jetpack plugin, update the icon font in the Jetpack plugin. This, of course, assuming the source repo tooling works in our favor (right now it doesn't, due to https://github.com/Automattic/social-logos/issues/116 )
  2. The icon font is problematic for display, and can cause issues in some scenarios: #6076, #13201, and #7148 are good examples.
  3. Relying on the SVG sprite in the Social Menu outputs all the SVGs on sites that may only need to display a few: #16673

As part of #27817, maybe we could switch all our implementations to one single npm dependency and single SVGs that would be used when necessary. cc @davemart-in

Issues that could be closed if this were addressed:

davemart-in commented 1 year ago

@jeherve what are you thinking would be the most efficient way to do this?

Let's label these for easy reference:

A) NPM dependency B) Icon font C) SVG sprite

Editing SVGs

As you know SVG code needs to be inlayed directly in the Jetpack's code if we want to be able to edit the colors dynamically (we can't just store the .svg files and call them directly). Using an icon font is another way around this, but similar to you, I've found them to be tedious, troublesome, and somewhat unreliable over the years.

I'm guessing that A & B both dynamically edit SVGs (i.e. change colors in CSS).

SVG Sprite

This one is easy. Since it's pulling directly from an SVG file, we know that there are no dynamic styles being set, as this is simply disallowed.

SVGs in React

In wp-calypso we store each SVG as individual JSX files which we can then include in any file. But this only works for React-powered interfaces.

Another approach would be to use SVGR. I'm not sure what the appetite would be to make this a JP dependency though. It certainly keeps things clean though since you can pull SVGs directly from SVG files and it will render them inline for you.

Icon font

This is likely going to be the trickiest one to accommodate. I see three uses of this font:

  1. admin-sharing.css
  2. amp-sharing.css
  3. sharing.css

Similar to SVGR, we can inline SVG's directly from an SVG file using PHP: <?php echo file_get_contents("{file}.svg"); ?>

But I'm not sure the best way for us to determine which icons would need to be loaded (to avoid having to load every icon on page load, whether they're used or not).

I'll stop here... :-) Curious to hear your thoughts.

davemart-in commented 1 year ago

Also, not sure where this comes into play, but I stumbled across it this morning, so I figured I'd call it out: https://github.com/Automattic/jetpack-social-icons

jeherve commented 1 year ago

I'm not sure the best way for us to determine which icons would need to be loaded (to avoid having to load every icon on page load, whether they're used or not).

I'm not 100% sure either to be honest. I don't have much experience with this.

avoid having to load every icon on page load, whether they're used or not

That, I think we should actively avoid.

Similar to SVGR, we can inline SVG's directly from an SVG file using PHP: <?php echo file_get_contents("{file}.svg"); ?>

I'm thinking this may be our best bet.

Each button has its own class, e.g. Pocket: https://github.com/Automattic/jetpack/blob/5463f2e0284ca8a634f75deee22db0c1bf1de984/projects/plugins/jetpack/modules/sharedaddy/sharing-sources.php#L2586-L2602

I'm thinking we could fetch the right SVG in each class, and load it for each button.

I stumbled across it this morning, so I figured I'd call it out: Automattic/jetpack-social-icons

Yep, this ended up in Jetpack. It lives here: https://github.com/Automattic/jetpack-social-icons/blob/92f1d6ed991ecafd1aed51adb8d338b260316284/jetpack-social-icons.php

It uses an SVG sprite, and thus suffers for that problem above: https://github.com/Automattic/jetpack-social-icons/blob/92f1d6ed991ecafd1aed51adb8d338b260316284/jetpack-social-icons.php#L68-L73 It's tracked in #16673.

jeherve commented 1 year ago

I'm thinking we could fetch the right SVG in each class, and load it for each button.

This may be our solution. Of course that would mean pulling the minified SVGs from automattic/social-logos into Jetpack: https://github.com/Automattic/social-logos/tree/97829b1d1416837ef9aee4df947b6cf3e4b410d6/svg-min

diff --git a/projects/plugins/jetpack/modules/sharedaddy/sharing-sources.php b/projects/plugins/jetpack/modules/sharedaddy/sharing-sources.php
index c0887f0536..43645baceb 100644
--- a/projects/plugins/jetpack/modules/sharedaddy/sharing-sources.php
+++ b/projects/plugins/jetpack/modules/sharedaddy/sharing-sources.php
@@ -329,8 +329,12 @@ abstract class Sharing_Source {
            );
        }

+       $svg_content = wp_remote_get(
+           esc_url_raw( plugins_url( '_inc/social-logos/svg-min/' . $this->id . '.svg', JETPACK__PLUGIN_FILE ) )
+       );
+
        return sprintf(
-           '<a rel="nofollow%s" data-shared="%s" class="%s" href="%s"%s title="%s" %s><span%s>%s</span></a>',
+           '<a rel="nofollow%1$s" data-shared="%2$s" class="%3$s" href="%4$s"%5$s title="%6$s" %7$s><span%8$s>%9$s</span>%10$s</a>',
            ( true === $this->open_link_in_new ) ? ' noopener noreferrer' : '',
            ( $id ? esc_attr( $id ) : '' ),
            implode( ' ', $klasses ),
@@ -339,7 +343,8 @@ abstract class Sharing_Source {
            $title,
            $encoded_data_attributes,
            ( 'icon' === $this->button_style ) ? '></span><span class="sharing-screen-reader-text"' : '',
-           $text
+           $text,
+           wp_remote_retrieve_body( $svg_content )
        );
    }
davemart-in commented 1 year ago

Let me recap what I'm hearing and you can let me know if I'm off:

A) We'd keep the NPM "social-logos" dependency for React code. B) We'd eliminate the icon font and replace it with a PHP class for each individual icon. This code would pull directly form the svg-min folder of "social-logos" and place SVG's inline (so that their styles can be dynamically updated). C) We'd use the same PHP classes as a replacement for the SVG sprite.

The only possible hitch in this plan might be instances where we currently use the icon font in say a background-image CSS property. I've not looked at the code in detail, but if we run into instances like this I suspect we'd either have to inline a <style> block or a style="" HTML property to get the SVG code inlined.

jeherve commented 1 year ago

A) We'd keep the NPM "social-logos" dependency for React code. B) We'd eliminate the icon font and replace it with a PHP class for each individual icon. This code would pull directly form the svg-min folder of "social-logos" and place SVG's inline (so that their styles can be dynamically updated). C) We'd use the same PHP classes as a replacement for the SVG sprite.

Yeah, that's the idea.

The only possible hitch in this plan might be instances where we currently use the icon font in say a background-image CSS property.

As far as I can tell, we're only using the icon font in one place in Jetpack: the sharing buttons, where we use the font in a :before pseudo element.

jeherve commented 1 year ago

Noting that the upcoming Share block will use svg from the Social Logos package: #27243