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 Icons: Preset colors do not stay in sync with Global Styles presets #35480

Closed
kjellr opened this issue Oct 8, 2021 · 6 comments
Closed
Labels
[Block] Social Affects the Social Block - used to display Social Media accounts Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended

Comments

@kjellr
Copy link
Contributor

kjellr commented Oct 8, 2021

When you assign a preset color to the Social Links block, it records the preset's hex color in the markup like so:

<!-- wp:social-links {"iconColor":"foreground","iconColorValue":"#000000"} -->

If someone were to go and modify the value of that foreground preset via Global Styles, the social links block does not update to the new color value. The original hex color defined in the block markup is still used.

GIF:

Note that I'm changing the color of "foreground" via Global Styles, but that update is not reflected in the Social Links block color:

social-links-color


Gutenberg version 11.7.0-rc.1
Noticed during the development of Twenty Twenty-Two: WordPress/twentytwentytwo#74 (comment)

@kjellr kjellr added [Type] Bug An existing feature does not function as intended [Block] Social Affects the Social Block - used to display Social Media accounts Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Oct 8, 2021
@kjellr kjellr changed the title Social Icons: Custom color does not stay in sync with Global Styles presets Social Icons: Preset colors do not stay in sync with Global Styles presets Oct 8, 2021
@ockham
Copy link
Contributor

ockham commented Oct 13, 2021

I don't recall the original rationale for the iconColor/iconColorValue pair, but maybe @aaronrobertshaw can help here? 😄

@noisysocks
Copy link
Member

noisysocks commented Oct 19, 2021

iconColorValue and iconBackgroundColorValue exist as fallback attributes so that the colour remains if you switch to a theme that doesn't have a colour named e.g. primary.

// Use custom attribute as fallback to prevent loss of named color selection when
// switching themes to a new theme that does not have a matching named color.
value: iconColor.color || iconColorValue,

The reason this bug happens is because Social Links passes the fallback attributes (iconColorValue and iconBackgroundColorValue) down as context instead of the primary attributes (iconColor and iconBackgroundColor).

"providesContext": {
"openInNewTab": "openInNewTab",
"iconColorValue": "iconColorValue",
"iconBackgroundColorValue": "iconBackgroundColorValue"
},

One fix for this would be to pass down both the primary and fallback attributes so that the Social Link block can set the colour to iconColor.color || iconColorValue.

Another fix could be to remove the fallback attributes altogether and only use the primary attributes. This is what #29565 does, which should fix this bug.

I'm not sure if the fallback is necessary? Do we do it for other blocks? It would be good if Social Links can use the attributes that setting supports.color adds.

@scruffian @oandregal @aaronrobertshaw

@aaronrobertshaw
Copy link
Contributor

Sorry I'm late to the conversation on this!

iconColorValue and iconBackgroundColorValue exist as fallback attributes so that the colour remains if you switch to a theme that doesn't have a colour named e.g. primary.

From memory, the issue regarding the loss of a named color selection when switching themes was raised in the review of the PR implementing the color options for the social links block. I don't feel strongly about maintaining this behaviour.

Another fix could be to remove the fallback attributes altogether and only use the primary attributes. This is what #29565 does, which should fix this bug.

The current colors implementation for the social links was added before we had the ability to skip serialization of block support styles.

There was plenty of discussion on the original PR around applying colors at an individual level vs once only on the primary social links block. It was the desire to have the colors set only once that ultimately drove the context-based approach. Initially, CSS variables were used to pass the colors down however that ran into some issues with KSES.

This block was a driving force in evolving the __experimentalSkipSerialization feature for block supports. It was intended to be moved over to block supports and #29565 is still listed in the tracking issue to handle that conversion.

It would be good if Social Links can use the attributes that setting supports.color adds.

Agreed. I'm happy to help out with reviews or pick up #29565 once 5.9 is released. I was under the impression that particular PR was still waiting on the ability to define custom labels but would need to follow that thread further.

@donnapep
Copy link
Contributor

donnapep commented Nov 15, 2022

Noting here that this is also an issue when you're trying to update the color of the icons in a style variation. Although iconColor is set to primary, the color doesn't update because iconColorValue is fixed.

@bradhogan
Copy link

+1 I don't understand why iconColorVaule is even an attribute. Or if it's going to be required, we should be able to set the value to something like var:preset|color|primary to stay consistent.

@ndiego
Copy link
Member

ndiego commented May 31, 2023

This issue is now fixed in #51020

@ndiego ndiego closed this as completed May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Social Affects the Social Block - used to display Social Media accounts Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

7 participants