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

Local block appearance controls should reflect values from global styles / theme.json#37752

Open
jameskoster opened this issue Jan 6, 2022 · 65 comments
Labels
[Feature] Colors Color management [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Needs Dev Ready for, and needs developer efforts [Type] Discussion For issues that are high-level and not yet ready to implement.

Comments

@jameskoster
Copy link
Contributor

jameskoster commented Jan 6, 2022

Current implementation

My theme.json declares a text color for h2s like so:

"color": {
    "text": "var(--wp--preset--color--grey-light)"
}

When I select a h2 in the Editor and open the color panel, I see this:

Screenshot 2022-01-06 at 14 57 58

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:

Screenshot 2022-01-06 at 15 00 48

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.

Inheritance option 8

  • Color: show the value directly in the color panel, instead of the "unset" color. In the flyout, show the transparent grid to indicate an unset color, but show below that the inherited color, in the format of "[Source]: [Color-name]". For starters, "Styles" is suffient as a source.
  • Typography: Show the inherited value as a gray ghost.
  • Sliders: Show the inherited value as a gray ghost.
  • Input values (border, radius, etc), show the inherited values as placeholder text.

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.

@jameskoster jameskoster added Needs Design Feedback Needs general design feedback. [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Discussion For issues that are high-level and not yet ready to implement. [Feature] Colors Color management labels Jan 6, 2022
@Mamaduka
Copy link
Member

Mamaduka commented Jan 6, 2022

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?

@jameskoster
Copy link
Contributor Author

Do you get the same results if you provide color value as HEX?

Yep, it's exactly the same.

@jameskoster

This comment was marked as outdated.

@karmatosed
Copy link
Member

karmatosed commented Jan 7, 2022

The canvas is telling me one thing, but the control is telling my something else.

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.

@karmatosed karmatosed added Needs Dev Ready for, and needs developer efforts and removed Needs Design Feedback Needs general design feedback. labels Jan 7, 2022
@carolinan
Copy link
Contributor

Isn't this true for all theme.json settings? That none of them are reflected in the corresponding control?

@aaronrobertshaw
Copy link
Contributor

aaronrobertshaw commented Jul 19, 2022

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 __experimentalStyles property on the store's settings object. This might now be limited only to the mobile context. There was an open PR exploring making these styles available within the block editor. I'm not sure if the approach there is still valid or if there is now a new method for accessing the merged global styles. Perhaps @jorgefilipecosta might have more insight here?

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.

@jameskoster
Copy link
Contributor Author

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.

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Sep 1, 2022

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.

when the color is inherited from a stylesheet (not necessarily theme.json). The control should account for those scenarios as well.

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.

@jameskoster
Copy link
Contributor Author

On #34178, I try to implement the case where we should the colors when they come from theme.json.

Thanks for the effort there. I would love to see the PR merged, but I understand it may be a gnarly one.

This would be complex.

For sure. We need further design exploration on how to handle raw styles. We can do this in #43082.

@jameskoster jameskoster changed the title Should the color control reflect default values set by the theme? Local block appearance controls should reflect values from global styles / theme.json Jan 13, 2023
@jameskoster

This comment was marked as outdated.

@jameskoster

This comment was marked as resolved.

@jasmussen

This comment was marked as resolved.

@jasmussen

This comment was marked as outdated.

@carolinan

This comment was marked as resolved.

@jasmussen

This comment was marked as outdated.

@jameskoster
Copy link
Contributor Author

That looks pretty good to me. Should we do something with the unit buttons? Currently there's no way to tell if they're set locally or inherited. Could they be grey when inherited like the value and blueberry when set locally? That would align with the range/segmented control treatment.

I know there aren't that many, but I'm also missing a design for toggles (e.g. drop cap).

@jasmussen

This comment was marked as outdated.

@jameskoster
Copy link
Contributor Author

Do we have any toggles that can inherit values?

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.

@richtabor
Copy link
Member

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:

CleanShot 2024-07-12 at 16 12 24

If it's too strong to have a color applied in the picker as well, an alt:

CleanShot 2024-07-12 at 16 13 44

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.

@jasmussen

This comment was marked as outdated.

@richtabor
Copy link
Member

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".

@jameskoster
Copy link
Contributor Author

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?

@richtabor
Copy link
Member

Could the source be something other than the global styles?

@fabiankaegy
Copy link
Member

Could the source be something other than the global styles?

Section styles maybe? 🤔

@jameskoster
Copy link
Contributor Author

Also maybe page-level or template-level styles in the future.

@jasmussen
Copy link
Contributor

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.

@richtabor
Copy link
Member

"Styles: Accent 1", perhaps with "Styles" in light gray?

Let's try it, to get this moving.

@jasmussen
Copy link
Contributor

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.

@jasmussen jasmussen added Needs Dev Ready for, and needs developer efforts and removed Needs Design Feedback Needs general design feedback. labels Aug 1, 2024
@ramonjd
Copy link
Member

ramonjd commented Aug 12, 2024

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.

  • In the editor, block style control components need to know about "inherited" styles. This could be done in BlockStyleControls.
  • Merged global styles settings, styles and other related properties are accessible in the block editor settings object, and therefore available in post/site editor contexts. The properties themselves are fairly disparate, __experimentalFeatures (settings), globalStylesData (styles) and so on. Probably room to consolidate them here.
  • These "inherited" styles to be passed to the block controls include not only merged base, theme.json + global styles, but also inherited style variations, element styles (any others?). I suppose WordPress will need some hook to fetch, merge and reconcile the hierarchy here.
  • Some inherited values need to be "resolved", the common example is a "ref" value in theme.json that refers to another value in the theme.json tree.
  • Resetting a block control's values will restore them to the inherited values. What about "removing" styles? Should a user allow unsetting inherited styles?
  • There are cases where a block needs to refer to its inherited settings/styles on the frontend, for example, using wp_get_global_styles() or wp_get_global_settings() in order to render its local classnames/styles. Layout does this already to some extent.

@jasmussen
Copy link
Contributor

Resetting a block control's values will restore them to the inherited values. What about "removing" styles? Should a user allow unsetting inherited styles?

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:

letter case control

@eirichmond
Copy link

eirichmond commented Aug 22, 2024

"Styles: Accent 1", perhaps with "Styles" in light gray?

Let's try it, to get this moving.

Could it be simplified further with just "Core", "Theme", "Global" or "Block" ?

@richtabor
Copy link
Member

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.

@eirichmond
Copy link

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.

Screenshot 2024-08-23 at 10 40 43 AM

Apologies if this is somewhat tangential to the original issue, but I believe it's related. Probably more relevant to #49278

@ramonjd
Copy link
Member

ramonjd commented Aug 25, 2024

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:

  • top-level styles
  • user global styles (the custom post type content)
  • element styles
  • block styles
  • block style variations

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Colors Color management [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Needs Dev Ready for, and needs developer efforts [Type] Discussion For issues that are high-level and not yet ready to implement.
Projects
Status: Needs Dev
Development

No branches or pull requests