-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Local block appearance controls should reflect values from global styles / theme.json #37752
Comments
I can't remember the issue/comment right now, but I believe the main problem is getting values from CSS variables. Do you get the same results if you provide color value as HEX? |
Yep, it's exactly the same. |
This comment was marked as outdated.
This comment was marked as outdated.
I second this expectation. If the canvas says something the control to me should reflect this. I have a feeling the sidebar hasn't caught this for a while as I seem to remember this disconnect being there for a while. However, I think it should get resolved as it feels less easy the further into site editing things go. I put on needs dev as to me the solution is fairly agreed on of showing the setting, but labels can always be adjusted from there. |
Isn't this true for all theme.json settings? That none of them are reflected in the corresponding control? |
I believe it is not just theme.json but also Global Styles. I'm not up-to-date on the inner workings at the moment but it was the case that the block editor doesn't get the merged theme.json/global styles in an accessible object within any data stores. That meant, short of inspecting every block and determining computed styles there was no way to determine what value a control should default to. If I recall correctly, the merged global styles were available in an Once we can access those style values, we could close the gap here between what we see in the editor canvas and the block's sidebar controls. Edit: It appears that global styles are now provided via context within the site editor. We might still need to apply a similar approach for the block editor. |
I think there's potentially another element to this that I overlooked – when the color is inherited from a stylesheet (not necessarily theme.json). The control should account for those scenarios as well. |
On #34178, I try to implement the case where we should the colors when they come from theme.json.
This would be complex. The stylesheet changing the color may not even be loaded on the editor. The markup on the editor may be a little different, making the colors different. But of course, we could try to should the current color rendered on the editor. |
Thanks for the effort there. I would love to see the PR merged, but I understand it may be a gnarly one.
For sure. We need further design exploration on how to handle raw styles. We can do this in #43082. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Good question. The only one I can find is 'Expand on click' on the Image block. But I appreciate the UI for that might change. |
I get a sense that seeing a background color applied on the Inspector, then reading "No color selected" is from the popover may be confusing. Reading "Inherited" may also be confusing for folks. I'm not sure this works, but perhaps something like this: If it's too strong to have a color applied in the picker as well, an alt: I kind of like the idea that we're connecting "Styles" here. Perhaps in a future iteration we could link to the relative style in global styles even. |
This comment was marked as outdated.
This comment was marked as outdated.
I'm not convinced "inherited" means something to most folks. Inherited from somewhere else? Where? I was trying to find a way to connect that this color is derived from somewhere else, by prefixing "Styles". |
I agree that the source of the inheritance will be important down the road. I don't know if it's necessary for a v1 of this, though? |
Could the source be something other than the global styles? |
Section styles maybe? 🤔 |
Also maybe page-level or template-level styles in the future. |
Initially it was the "Styles/Accent 1" that didn't gel with me. That sounded like the name of a single color. But the idea of the prefix works, so maybe it's just a visual thing: "Styles: Accent 1", perhaps with "Styles" in light gray? Can even start there, and then see if we need further clarification, and then ambiguate into "Global styles:" and "Section styles:" as proposed. |
Let's try it, to get this moving. |
Alright folks, based on the last few items in this conversation, I've again updated the mockups (here in Figma) based on the latest. I've also updated the issue itself to reflect this, and in the spirit of getting this in motion for 6.7, going to now swap out the labels. |
I've been meddling in this area while working on background image values, and noted some technical implementation details/questions. I'm adding them here for reference.
|
Yes, but this is a UI challenge that's already present in the block editor, in fact showing the inheritance is a first step to helping make it clearer where you might be able to unset an inherited style. One common place where we've solved it is inherited text case, where we added a value for the default (the minus) here: |
Could it be simplified further with just "Core", "Theme", "Global" or "Block" ? |
All that matters is that this is inherited; that this color is not applied at the block level. Whether or not it's a core, global, or theme color is not relevant—perhaps even confusing. I was suggesting "Styles" as it's the UI source for all styles. |
Thats a fair point and I agree—I see your point about "Styles" being a good starting point. However, this brings up a broader conversation from a developer's perspective. For me, these distinctions (Core, Theme (or Styles 😇 ), Global or Block) are relevant and, at times, can be confusing. For example, in core, the blockGap is set to 24px, but in the UI, it appears as 0px until you interact with it. To maintain the original core look, the user has to manually set it to 'X-Small' at the block level. Displaying something like "Core, Styles, Global, or Block" could help reduce confusion from a development standpoint, in my opinion. I’ve attached an example with three columns to illustrate how blockGap interaction works. The same issue occurs with the button outline variation, where the border is set to 2px but isn't shown in the UI. Apologies if this is somewhat tangential to the original issue, but I believe it's related. Probably more relevant to #49278 |
We've started looking at representing a block's inherited values in the controls in this tracking issue: Feedback/ideas are welcome, though it's early days 😄 and there's nothing yet to test. Initially, the work will be focussed on drilling through theme.json/global styles data arrive at the right value. For example, assuming only theme.json/global styles as a source, a block could inherit from:
I'm not any of these labels are helpful to someone who just wants to style a block, unless, it's a theme developer wishing to know where the inherited styles are coming from (?) Thanks! |
Current implementation
My
theme.json
declares a text color for h2s like so:When I select a h2 in the Editor and open the color panel, I see this:
The canvas is telling me one thing, but the control is telling my something else. I can clearly see that a color is set, but the 🚫 icon in the control makes it look like no color is set. When I open the color picker, the transparent texture confuses things further by suggesting that the color is set to
transparent
. This combines to erode my confidence on what will actually appear on the frontend.Expectation
My expectation would be to see something like this, where the control reflects the code exactly:
Suggestion
We show all values that are inherited from theme.json / Global Styles. Here's an example showing the Button block as styled globally, showing its values — colors, font size, padding, radius — in the inspector even locally.
This would be a first stap that doesn't necessarily preclude further steps taken in #49278 to visualize where the inheritance is coming from, but it might also reduce the issue enough that it'd be less urgent, perhaps even irrelevant. It would also make clearer when the contrast notice appears or doesn't; at the moment the values compared for contrast are not aware of inheritance, but with this change they could be made to.
Issue updated Aug 1, 2024.
The text was updated successfully, but these errors were encountered: