Social Icons: Add icon & background color options #28084
Conversation
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: -18.2 kB (-1%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This works well for me and I would agree I prefer this implementation over #25372
I didn't have any problems with implementation and usage, I would defer to @jasmussen for any comments on the CSS change, it looks like it just wraps it in a not if specified which worked fine in my testing, so I don't see an issue.
Good work on this, thanks! 👍
|
This is awesome. Theme developers are going to be so happy with this. Here's what I see:
A couple of issues here: 1. I'm using TwentyTwentyOne Blocks and for whatever reason, setting the global color of the entire Social Icons block doesn't work. These appear to be the conflicting styles: You note:
We have to solve this. It'd be okay to increase specificity of the user-set colors. Setting the selector to this, solves it for me:
Just add a CSS comment explaining why the extra specificity is added. 2. It's possible to override the color of just a single icon. My very first instinct here is that this is a step too far. The color interface is identical whether you've selected the Social Icons parent block, or any individual block inside, meaning users will likely either set mixture of both, and be confused when one overrides the other. An additional instinct says that the use case of coloring only a single icon, as opposed to all of them at once, is an exceptionally rare use case that doesn't warrant the UX overhead. I'm happy to defer to a sanity check by @jffng, @MaggieCabrera, @kjellr or @scruffian, who I believe have been yearning for this feature for a along time. If they prefer both the block itself, and individual social icons, to be colorable, I'll happily defer. Thank you again for working on this 🏅 |
|
Thanks for the ping, @jasmussen! And thanks for the PR, @aaronrobertshaw! This is definitely a feature I'm excited to have. 🙌 I fully agree with @jasmussen's assessment, both about adding specificity to the user's color choice, and also the stance that these color options should apply only to all social links. |
@jasmussen I can't replicate the issue of the global color selection not being passed to the child Social Link blocks and being applied. The CSS styles in the screenshot also appear different than those I see.
I'll try and address this. The catch is the In the previous PR I overcame this by actually determining the color value and applying it as an inline style. This lead to a somewhat more hacky approach and a lot of additional changing of code that others weren't a huge fan of.
That was my preference as well to only set the colors at the Social Links level as a group. Again, this was something that was covered in the original PR. The approach in this PR uses the colors block support to be able to apply the color CSS classes etc to the individual blocks. The controls added at the "group" or Social Links level are simply used to pass through values to set the individual blocks' color support attributes. I'm not sure if I can disable/hide the color controls at the individual block level but still retain the color block support behaviour. I'll look into it. There's also a possibility that I can leverage some CSS variables to find some middle ground. |
|
I've updated this to use a new approach leveraging CSS variables set on the main Social Links block. Improvements over the last iteration are:
One downside was needing to store the color hex value for named colors in an attribute so the CSS variable inline style in save.js can still be correctly set. PR description has been updated to reflect current approach. @jasmussen and @kjellr would you mind taking another look at this and seeing if it is more to your liking? 🙂 |
|
Awesome work, still. The removal of the ability to color individual icons greatly simplifies the experience. However I still can't get the colors to take:
They need relatively high specificity. Which theme are you testing on? Also, I'm not sure moving to CSS variables for these is a good idea. You might want two social links menus on a single page, with different color sets. For example you may have a light header with a couple icons at the end of your navigation menu, which are dark gray against the white background. Then you might also have social links showing up at the end of every post, with different colors. Won't the move to CSS variables make the choice of colors for one social icons block affect the colors of every other social icons blocks? |
|
Hey 👋 - Thanks for working on this! I'm wondering why you didn't try to implement with I think it would be much simpler and would need just the addition of a bit of css. I haven't checked it yet though, but I will as I am curious :) --edit I missed the fact that you wanted to change all colors at once :) |
I tested with TwentyTwentyOne (as per the last gif) using latest master, v1.0, and v1.1, TwentyTwenty, Maywood, and Varia. Prior to trying to replicate the issue you've seen, I updated all the themes as well. I also tested across browsers and still can't replicate the problem.
This is actually part of the beauty of the CSS variables approach, that will not be the case. CSS variables can be inherited or scoped to a specific selector not just set globally on The current implementation I have in this PR sets the CSS variable as an inline style on the block. When the inner elements' CSS attempts to determine the variable's value via Below are a few GIFs with multiple social links blocks using different color selections.
|
If switching themes, their color palettes may not have matching named colors. In this situation, the previous color selection's named color would not match an option in the color controls so they would effectively clear the selection. We are already maintaining a custom color value attribute to facilitate setting CSS variables in the save function. Using this instead of the withColors created color means if it does match we keep the selection. If it doesn't it is treated as a custom color.
This partially undoes the prior commit that switched all color selections to use the custom attribute value. Instead, by using them as a fallback only, if the named color is also in the new theme's palette, that themes colors are used.
|
I suspect the specificity issues are related to none of those themes being block based, and potentially not leveraging some of the global styles stuff that's underway. I can confirm it works in non block based themes, but falls apart in block based ones. You can try using the block based themes in https://github.com/WordPress/theme-experiments. Here's my own:
Here's Twenty Twenty One Blocks:
Here's a video of a little inspecting: https://cloudup.com/cxeNNecoxC0 As you mentioned, it works in vanilla Twenty Twenty One:
I can even see the right styles taking:
☝️ that CSS appears to not be output at all in block based themes. |
|
@jasmussen Thanks for your help and patience on this one.
I've tested with the themes from https://github.com/WordPress/theme-experiments including:
All of them work fine for me with this PR (Twenty Nineteen Blocks has broken layout that's a separate issue). The closest this PR now comes to Global Styles is using the The color selections via the CSS variables are applied to the individual Social Links via simple CSS rules added to The CSS classes applied to the group social links block and the individual link blocks match between your video and what I see. Double checking the CSS rule, it should be applied so long as its in your stylesheet. I've also cloned a fresh copy of Gutenberg, setup a new wp-env install, manually uploaded the block themes, rebuilt Gutenberg and re-tested. Same result, they all work for me. I'm obviously missing something.
|
|
My goodness. I had Then I opened a new tab, switched the theme to TwentyTwentyOne Blocks, opened the same post in the post editor, and saw that the colors did not work. Then I debugged the two open tabs, without reloading, to figure out that the CSS to colorize each list item was simply missing in the tab that had TwentyTwentyOne Blocks. I even tried clearing cache thoroughly. In a bit of frustration, I Ctrl+C'ed my I still don't quite understand why the existing background |
On some rare occasions I've seen the my local watch task see the .scss files change but not build correctly when switching branches. I've not thought much of it at the time and have probably developed a bit of a habit of restarting
That one is a bit of a mystery!
Not a problem. I'm glad it's working for you now. If nothing else, we've been pretty thorough testing this one across themes 🙂 |
|
This one will make a lot of people happy. As a followup, we should still make the change where we move the color options to the toolbar, as shown in this comment. Aaron if you have energy of course, otherwise CC: @mkaz? Alternatively we can open a new ticket to do just that, so it doesn't fall off the radar. |
How do you see the two different color options working in the block's toolbar? We have both icon color and icon background color. Do we need to combine those into a single toolbar menu? |
Something along these lines:
Some of this is a little dependant on work in #27331 to further. |
|
I'll open a ticket. |
| @@ -348,6 +348,7 @@ These are the current color properties supported by blocks: | |||
| | Post Title | Yes | Yes | - | Yes | | |||
| | Site Tagline | Yes | Yes | - | Yes | | |||
| | Site Title | Yes | Yes | - | Yes | | |||
| | Social Links | Yes | - | - | Yes | | |||
There was a problem hiding this comment.
👋 While this PR added support for icon color & background, it doesn't use the block support mechanism, hence these properties can't be targeted via theme.json. I've prepared a fix for this at #29294
There was a problem hiding this comment.
Thank you for the fix!
|
👋 I've filed an issue at #29297 that relates to the use of CSS Custom Properties in this PR. |
Fixes: #21605
Description
This is an alternate approach to #25372 using CSS variables on the primary Social Links block that are then used by styles on the individual Social Link blocks.
Compared to the original approach this PR:
How has this been tested?
Manual Testing.
Screenshots
Types of changes
Enhancement
Checklist: