-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Consolidate "bound block" color and "synced" colors #60617
Conversation
As an aside, I'm not certain the icon should be added to the toolbar in the first place. This effort is still relevant, regardless of how the UI for expressing a bound block/override changes. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +58 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we need to change the CSS variable here as well?
gutenberg/packages/block-editor/src/components/block-list/use-block-props/index.js
Line 133 in 92c6963
? { '--wp-admin-theme-color': 'var(--wp-bound-block-color)' } |
@@ -5,5 +5,4 @@ | |||
@include admin-scheme(#007cba); | |||
--wp-block-synced-color: #7a00df; | |||
--wp-block-synced-color--rgb: #{hex-to-rgb(#7a00df)}; | |||
--wp-bound-block-color: #9747ff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for changing the colors, I think it's okay, but the base-styles
package itself is published as an npm package, and developers may import it. Although this CSS variable was recently added, I don't think it should be removed. Otherwise, if the developer had defined a style like this, the colors would be lost.
@import 'node_modules/@wordpress/base-styles/default-custom-properties';
.element {
color: var(--wp-bound-block-color)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be removed. Otherwise, if the developer had defined a style like this, the colors would be lost.
What wiggleroom is there for errors, which this clearly was? I'd tend to think that with a proper changelog note it should be possible to deprecate this just like other JS pieces can be deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My approach is to leave the CSS variable and change it to reference the recommended CSS variable.
:root {
--wp-block-synced-color: #7a00df;
// This CSS variable is not used in Gutenberg project,
// but is maintained for backwards compatibility.
--wp-bound-block-color: var(--wp-block-synced-color);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 1c09419
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've researched this comment, and indeed the old CSS variable needs to be updated. Otherwise, the outline will not be visible when any source is bound to the block. trunk: This PR: By entering the following HTML in the code editor, you can test the block to which the source is bound. <!-- wp:paragraph {
"metadata":{
"bindings":{
"content":{
"source":"core/post-meta",
"args":{
"key":"book-genre"
}
}
}
}
} -->
<p></p>
<!-- /wp:paragraph --> |
Added in 9e89ece. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
What?
A piece of #60286.
Consolidate
--wp-bound-block-color
and--wp-block-synced-color
into one variable. They're just slightly different purples, where it's not really necessary. Synced patterns use--wp-block-synced-color
already, perhaps bindings should just use that too. #60599 leans into--wp-block-synced-color
for overrides as well.Testing Instructions
Screenshots or screencast