Add link color component #22722
Add link color component #22722
Conversation
|
Size Change: +353 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
57c14fc
to
1ae7dbd
Compare
a415c69
to
b60733d
Compare
1ae7dbd
to
79a8437
Compare
fb5c964
to
d35b123
Compare
|
👋 Jorge. This is great work put together very quickly, appreciate it! ❤️ 🌟 There's a few things in this PR, I'm trying to address them separately so feedback is actionable. 1. How user modifications at the block-level should work? I believe we have a few options here
I think we need to build a common understanding about how this should work to have a direction to walk towards. In the conversations we had at the beginning of the year, my understanding was that user choices at the block level should persist theme switches and should take precedence over theme styles (global-level). Following that thinking, it seems that we should implement the first option. 2. How do you see link color statuses (hover, visited, active) fit into this picture? I'm not sure how these fit with what we currently have (these can't be expressed as inline styles). I also wonder how this and the first point play together. @ItsJonQ has brought up this a few times, so pulling him here in case he has any suggestions. 3. Can we decouple block-level & global-level for this PR? I see this connects both levels using I suggested before that we should delay this connection until the pieces were mature, so the Global Styles feature can remain experimental for WordPress 5.5. Doing this now is forcing us to open-up Global Styles to production without having given theme authors time to give feedback, and we act on it. As of today, I feel we should keep Global Styles experimental for WordPress 5.5. 4. Should we normalize the CSS variable names? The other variables we use follow this pattern This needs rebase to make it work with block.json supports (as well as global supports). |
|
Hi @nosolosw, thank you for reviewing this PR and sharing important questions!
This discussion is how to reference predefined colors. The same discussion was also happening at #21490 (comment). I will share my thoughts here. I agree that there are three options as @nosolosw shared: Direct value, reference a CSS var, or class. Regarding this disadvantage, "when the user changes themes, the block style is broken because the variable is not in scope". If predefined colors are kept and the theme is changed, the old colors may not look good at all on the new theme, and the design may appear broken as well. There may be cases where the design may look even more broken if predefined colors are kept than if the defaults (no color selected) of the new theme are applied. This PR uses CSS vars because, in previous discussions, it seemed we decided to change the direction from class usage to CSS variable usage. It was this change of direction that made me propose #21490 to follow the new direction.
This is a tough question. It relates to the discussion of variable reference vs. classes. If we use variables, there are not many options available. There are ways to use calc and compute a color based on other colors, but it would only work if we use variables for red, green, blue, or luminosity, saturation, etc. and making themes set colors in this way is not practical. At first sight, the pseudo-elements in theme.json make more sense. And I don't see a blocker for the theme.json engine to support them.
I think this piece is even more important than the link color itself and allows us to test global styles in ways that were not tested before. E.g., Have the blocks show default values that come from theme.json have the blocks show values inherit from the parents, etc.
Global styles are experimental. The files have the experimental prefix, it is common that we merge experimental features in core, so I don't see why global styles should be different and can not have experimental functionalities included.
I agree, and to me, it seems global styles are still experimental. The JSON files that set them have the experimental prefix. This PR is not adding anything to the public API that does not has the experimental flag (besides the link color style attribute) and was mostly implemented as a plugin. If we don't port the PHP files to the core that parse and include theme.json information, theme.json will not be able to change or affect the editor. What in this PR does not looks experimental and should be fixed?
Yes, thank you for catching this. I think we should update the CSS variable name. I will change it to --wp--style--color--link. |
Although I have a preference for the first approach I shared, I understand that following any of the other options is how things work today, so I'd be fine merging this as it is. Perhaps when we have the global styles UI we can revisit this decision if it makes sense.
👍 What you say makes sense, thanks for sharing. I agree it shouldn't stop this PR from being merged.
In this point, I believe we agree on the need to do this and that doing it will teach us valuable lessons. It seems we disagree when it comes to decide the time & space: whether or not we need it for this Pull Request. I still think that a better space for testing the grounds of deep editor integration would be the global styles sidebar at the edit site. Note that this editor integration is not really required for the link color feature to work! If we decouple block & global levels, it'll work as fine as the other features we implemented and that we'll ship with WordPress 5.5 (colors, font sizes, line height). For example, we could do something like: a {
--wp--color--link: <value>;
color: var(--wp--color--link);
}In my view, it'd be a nice and scoped experiment (a CSS variable that is specific to links and scoped to I reckon I may be conservative in this point, and I'll trust more experienced voices on this. I just want to make my rationale as clear as possible, which is hard in such a nuance topic: the reason I'm conservative is that I've seen that when something lands in WordPress core, it's really difficult to remove/update it (no matter the name starts or not with experimental). So, if something is not really necessary, should we allow ourselves to perhaps be a bit more prudent and wait to integrate this in the next WordPress cycle? |
d35b123
to
f5fd0f6
Compare
e5433e5
to
fed69d4
Compare
|
Following a @nosolosw suggestion, this PR was updated to include a has-link-color class so we make sure we don't break the current link color for existing themes. When we allow link color to be set via theme.json we will need to stop relying on the class because in that case, a block may have a link color set in. theme.json without having the class in their markup. |
fed69d4
to
59ce612
Compare
|
I've tested this with 2020, 2019, and 2017 themes and this is working as expected:
I think the only bit left would be to make this work with themes that do support theme.json. I've tested with https://github.com/nosolosw/global-styles-theme/pull/8 We could, for example, add in if ( gutenberg_experimental_global_styles_has_theme_json_support() ) {
// To support all themes, we added in the block-library stylesheet
// a style rule such as .has-link-color a { color: var(--wp--style--color--link, #00e); }
// so that existing link colors themes used didn't break.
// We add this here to make it work for themes that opt-in to theme.json
// In the future, we may do this differently.
$css_rule = 'a { color: var(--wp--style--color--link, #00e); }' . "\n";
}I think we can land this PR after that's addressed. |
59ce612
to
45cfd6f
Compare
|
Hi @nosolosw thank you for the suggestion, I included that code. |
45cfd6f
to
ffe4f0b
Compare
ffe4f0b
to
784abc4
Compare
|
Thank you for all the reviews and insights @nosolosw! |
|
Created #22929 to document this feature. |
|
@jorgefilipecosta now that global-styles.php needs to be loaded for every request, would you mind filling me up on what would we need to do to port this to WordPress core? Is this something that's done during the release or should we create a patch now? |
|
The backporting to core essentially is implementing the same functionality we have has a plugin into core. When backporting things to core it does not means the functionality is implemented as in the plugin. E.g: instead of using a filter, in core it may make more sense to change directly the place where the filter is executed. The code should be equivalent to what we have in the plugin but should not be the same it needs to look like it was built in core first |
|
As referred in #22929 (comment). If we think we are not ready to have the global styles loading in the core we have some options.
|
|
As per #23210 (comment) this isn't going to be part of 5.5 and it requires the plugin to work. Discussion about this has been scattered through issues and reviews, so I'm listing them here for clarity/awareness:
|
Depends on: #22744
This PR adds link color picking UI and the ability to configure link color using theme.json.
The following blocks get a filed to pick the link color:
When the variable from theme.json is changed the UI and the new default link color is updated.
This PR has an issue it may change the default link color for existing themes. I don't think we can avoid that, the only option would be to not show the link color UI at all if the theme does not use theme.json
How has this been tested?
I verified the default link color is #00e and the UI of the blocks shows that.
I added the following code data to :
I verified the default link color of the paragraph starts being red while for other blocks is still "#00e".
I tried to change colors in groups and verified the colors propagate correctly to the descends.