Update: Use elements mechanism for link color instead of a CSS variable #31524
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgefilipecosta
added
the
Global Styles
|
Size Change: +46 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
update/link-color-mechanism
branch
from
cb8a78f to
30e1f1f
Compare
jorgefilipecosta
requested review from
ellatrix,
oandregal,
spacedmonkey and
TimothyBJacobs
as code owners
oandregal
reviewed
May 7, 2021
oandregal
reviewed
May 7, 2021
oandregal
reviewed
May 7, 2021
jorgefilipecosta
force-pushed
the
update/link-color-mechanism
branch
2 times, most recently
from
a9eb5c8 to
04039af
Compare
|
Hi @nosolosw, thank you for the review your feedback was addressed. Meanwhile, this PR also got more complex because we were not dealing with some cases. The layout and duotone hooks assume there is always a class attribute present. In this case, we can not make that assumption (e.g: a simple paragraph). So I added code to deal with these cases where our code does not only needs to add a class but also needs to add the attribute. |
oandregal
reviewed
May 7, 2021
oandregal
reviewed
May 7, 2021
oandregal
reviewed
May 7, 2021
oandregal
reviewed
May 7, 2021
oandregal
reviewed
May 7, 2021
|
In "styles": {
"elements": {
"link": {
"color": {
"text": "#00E"
}
}
}
} |
oandregal
reviewed
May 7, 2021
oandregal
reviewed
May 7, 2021
oandregal
reviewed
May 7, 2021
oandregal
reviewed
May 7, 2021
|
I've done the first pass at this: the direction is good! Excited to have this in. Some thoughts:
|
jorgefilipecosta
force-pushed
the
update/link-color-mechanism
branch
3 times, most recently
from
6fd8602 to
f3c4da0
Compare
oandregal
reviewed
May 18, 2021
oandregal
reviewed
May 18, 2021
oandregal
reviewed
May 18, 2021
oandregal
reviewed
May 18, 2021
|
I'm testing with existing content, to see how this behaves. This is what I've tried: Using the
<!-- wp:paragraph {"style":{"color":{"link":"var:preset|color|red"}}} -->
<p class="has-link-color" style="--wp--style--color--link:var(--wp--preset--color--red)">
Paragraph with <a href="http://wordpress.org">link</a>.
</p>
<!-- /wp:paragraph -->Switch to this branch:
<!-- wp:paragraph {"style":{"color":{"link":"var:preset|color|red"}}} -->
<p class="has-link-color">
<p class="has-link-color" style="--wp--style--color--link:var(--wp--preset--color--red)">
Paragraph with <a href="http://wordpress.org">link</a>.
</p>
</p>
<!-- /wp:paragraph -->
<p class="wp-elements-60a3b78a854cd has-link-color"></p>
<p class="has-link-color" style="--wp--style--color--link:var(--wp--preset--color--red)">Paragraph with <a href="http://wordpress.org">link</a>.</p>
<p></p>
<style>.wp-elements-60a3b78a854cd a{color: var(--wp--preset--color--red);}</style>
<!-- wp:paragraph {"style":{"color":{"link":"var:preset|color|red"},"elements":{"link":{"color":{"text":"var:preset|color|red"}}}}} -->
<p class="has-link-color">
<p class="has-link-color" style="--wp--style--color--link:var(--wp--preset--color--red)">
Paragraph with <a href="http://wordpress.org">link</a>.
</p>
</p>
<!-- /wp:paragraph -->I think we have a couple of issues here:
|
|
Ok, I've done a second pass at this. The major issues I've found are:
|
Hi @nosolosw, on the backgroundColor and textColor, we have two attributes one for named colors and one just for values so we can differentiate between color value "red" and present color "red". I think the system we have for link color is much much better. We use a single attribute. The shape of the style attribute is exactly the same as the shape of theme.json. When using a different shape people will be confused: Why on theme.json to set a paragraph link color I use style.elements.link.color for presets and color values but when creating a pattern to style a paragraph I use style.elements.link.color for color values but If I want to use a preset I need to use a linkColor attribute that is not available on theme.json? |
jorgefilipecosta
force-pushed
the
update/link-color-mechanism
branch
from
79d57c1 to
0c55f8d
Compare
jorgefilipecosta
force-pushed
the
update/link-color-mechanism
branch
from
0c55f8d to
4bc2ac2
Compare
|
Hi @nosolosw I applied your suggestions to be compatible with current theme styles and I mitigated the back compatibility issue (we are not breaking the markup of the paragraph anymore). |
oandregal
force-pushed
the
update/link-color-mechanism
branch
from
4bc2ac2 to
4b9438f
Compare
|
Ok, now this works in the tests I've done:
However, I'm a bit concerned about the resulting markup of existing content. Steps for testing:
<!-- wp:paragraph {"style":{"color":{"link":"var:preset|color|red"},"elements":{"link":{"color":{"text":"var:preset|color|red"}}}}} -->
<p class="has-link-color" style="--wp--style--color--link:var(--wp--preset--color--red)">
Paragraph with <a href="http://wordpress.org">link</a>.
</p>
<!-- /wp:paragraph -->Note how the legacy mechanism for block attributes is still there ( Do you think we can ship this as it is and do the migration in a follow-up? Do you think we could modify this PR to use the existing block attribute |
|
These are the blocks that have support for link color and, hence, would need to be migrated: column, columns, group, heading, media-text, paragraph, post-author, post-comments, post-comments-form, post-comments-link, post-date, post-excerpt, post-terms, post-title, query-pagination-next, query-pagination-previous, site-title, template-part, term-description, verse. I'm particularly concerned with the inline styles, as that could cause styling issues with themes that use the custom property. Perhaps unlikely for the majority of cases given our use of Given the time constraints to get this in the RC, a compromise we could do is to ship this PR as it is and backport those migrations individually later ― if that's something folks agree with or makes sense in general. |
|
Ran this and this by some folks. Given this has been an experimental feature not shipped to core, there's going to be a small number of users that need those migrations. The consensus is that this is an important PR to have in today's RC and that we can fix bugs later should they happen. Going to disagree and commit with this one. |
oandregal
approved these changes
May 19, 2021
jorgefilipecosta
force-pushed
the
update/link-color-mechanism
branch
from
4b9438f to
ac7e6da
Compare
jffng
pushed a commit
that referenced
this pull request
May 19, 2021
…le (#31524) Co-authored-by: André <nosolosw@users.noreply.github.com> (+3 squashed commits)
15 tasks
This was referenced Sep 9, 2021
2 tasks
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the link color mechanism to use elements instead of CSS variables.
The attribute is persisted in a paragraph is the same as in theme.json (inside elements). This is the first time we use an elements object at the block level. So we are introducing a new mechanism.
The shape looks like this:
The mechanism on the client-side is generic, e.g., it would work for any style and any element. The mechanism on the server is specified for link color for now. We can make it generic, but it requires exposing a method from lib/class-wp-theme-json.php that compiles the styles. To avoid having this PR change the public API of class-wp-theme-json.php, I removed the generic approach and kept things simpler and specific for link color so we can merge this PR faster.
How has this been tested?
I went to the site editor global styles panel and set a global link color and a link color for the heading block.
I verified both link colors were applied correctly on the front and backend.
I went to the post editor and set a red link color for a paragraph. I verified the link color was applied correctly on the editor and frontend.