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

Social Links: Update SVG rendering to use CSS and background mask #19888

Closed
wants to merge 3 commits into from

Conversation

mkaz
Copy link
Member

@mkaz mkaz commented Jan 25, 2020

Description

The goal of this PR is to allow the Social Links to be extendable, so a third-party could write a plugin to add their own icon and link. A couple issues need to be resolved to make it extendable:

  1. Use a single icon for all sites in the inserter

One of the issues with social links is a duplication of the icons, one copy for displaying within the content and another for displaying in the inserter. This PR switches the inserter to use a single external share icon for all the sites.

  1. Use CSS background image and mask to insert the SVG via a URL.

The second issue is duplication of SVG between PHP and CSS, if we switch out the way we render to use a single SVG file, then we can use that file to display the logo and using background mask, it can handle the circle with white icon case. This allows moving all SVG definition to CSS, though requires a location for these SVG files so the front-end can access.

  • Does not work for logo only style
  • Duplication of sites and colors, should be a way to use mixins to solve
  • Need a static location to refer to the SVG files, external url used for testing.

With the last update solved two of the issues, working with logo-only style and duplication of sites/colors.

The remaining outstanding issue is finding a static location to put the SVG files. These should be distributed with the Gutenberg plugin, or WordPress core. My suggestion is to create a package that copies the files to gutenberg/build/social-icons that the plugin can refer to. Then for WordPress core to include in the wp-includes/images directory, which is where other files assets are stored.

An alternative solution to this PR: use all SVG in the post content, switch everything to use JS/SVG component but due to KSES this would requiring restricting the block to just admin roles.

@@ -52,7 +52,7 @@ const SocialLinkEdit = ( { attributes, setAttributes, isSelected } ) => {
className={ classes }
onClick={ () => setPopover( true ) }
>
<IconComponent />
<em />
Copy link
Member

Choose a reason for hiding this comment

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

Semantically, I don't think an em is the right element to use here. Why not a span?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, let's move to a span. Em was perhaps a bit too clever.

}

.wp-social-link-mastodon {
@include sociallink( "https://cldup.com/a2oE4qAWQz.svg", #3288d4 );
Copy link
Contributor

Choose a reason for hiding this comment

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

We obviously want this stored somewhere other than Cloudup.

Outside of jumping through s.w.org shenanigans, can't we put these in wp-includes/images? Maybe not as part of this PR, but it seems the appropriate place for it, and a good conversation to have.

@jasmussen
Copy link
Contributor

Marcus, thanks for this. This is a good PR, and it's an excellent way to have a conversation about how to move Social Links forward. As it stands, this PR solves a number of problems:

  • No SVG duplication
  • SVG could be a sprite if we really mean for it to
  • It works in modern browsers, and if not this PR, the codepen linked works in IE11 by making the icons black and white.

Those are significant challenges with the current system, and they serve as blockers for adding extensibility, and honestly, even just graduating this from being experimental.

Keep that in mind as you look at the following tradeoffs. The singular icon in the block library feels a little weird at first glance:

Screenshot 2020-01-27 at 09 37 00

Are we okay with IE11 just getting black and white?

screenshot_2019-10-14_at_11 13 35

I'll be adding a design feedback label, as it's worth accepting those tradeoffs before moving forward.


My thoughts on the two tradeoffs.

A single icon

With the current block library layout, this is a bad experience. However a separate ticket suggests a different layout:

Screenshot 2020-01-27 at 09 47 50

In this horizontal layout, we could even go with no icon, or the most generic of icons. There is already precedence for this in our embeds, and it hasn't been brought up much:

Screenshot 2020-01-27 at 09 49 44

IE11 in black and white

Completely fine with this. IE11 is barrelling towards obsolescence and deprecation, and from just a technical point of view it's important to not hold back significant features based on a low, not even common, denominator. Additionally, the fallback: fully legible black and white icons, is fully usable and accessible, so it serves as decent support.

@jasmussen jasmussen added the Needs Design Feedback Needs general design feedback. label Jan 27, 2020
@mkaz mkaz changed the title Try to fix up Social Links Social Links: Update SVG rendering to use CSS and background mask Feb 6, 2020
@mkaz mkaz force-pushed the try/update-social-links branch from dc5731d to 23f7c59 Compare February 7, 2020 18:44
@mkaz
Copy link
Member Author

mkaz commented Feb 7, 2020

@jasmussen I updated using the background mask, this works fine for the color circle and white logo, but does not work with logo only. From what I've tried the external SVG file can't be colored with the service's color, they all end up black & white.

This again is still just a draft, I only converted three sites to external URLs, I tried a couple of different ways to embed the SVG in the post; data encoding using utf-8 or base64 didn't work. We run into the same KSES issue for publishing SVG. You need to test as an Author role, not Administrator.

@mkaz
Copy link
Member Author

mkaz commented Feb 8, 2020

I have a solution if we can find a home for a directory of SVGs to deploy with Gutenberg. Right, now it looks like the other icons used across Gutenberg are embedded within the JavaScript, from what I can find there is no stand-alone assets.

Another (not ideal) solution is to avoid stand-alone files we can create a CSS sprite that embeds all of the SVGs in the CSS using data-urls. My estimate is that it will be around a 48kb CSS file.

@jasmussen
Copy link
Contributor

Nice, Marcus. That solution, though, does it account for extensibility? I.e. 3rd parties registering additional social links?

@mkaz
Copy link
Member Author

mkaz commented Feb 10, 2020

@jasmussen Yes, with the change it will be possible to extend using a plugin. The plugin wil need to include a new SVG, a bit of CSS to match, and a JS function to add a new variation. We'll need to confirm and document how, but should be possible.

I pushed up the changes, to get it to work for testing. Extract this social-icons.zip to the Gutenberg build directory. These are the files we need to find a home for, it would work for the Gutenberg plugin but also need to do something for core install.

After extracting you should have a directory: gutenberg/build/social-icons/ with the SVG inside. You could then try the updated patch.

@mkaz mkaz mentioned this pull request Feb 26, 2020
5 tasks
@mkaz
Copy link
Member Author

mkaz commented Apr 13, 2020

Closing this one out, we can reopen and revisit at a later point.
See #19041 that highlights a few different approaches to explore.

@mkaz mkaz closed this Apr 13, 2020
@mkaz mkaz deleted the try/update-social-links branch April 13, 2020 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants