Global Styles: Improve organisation of site background color and image #63216
Conversation
|
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: +2.61 kB (+0.15%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
shail-mehta
added
[Type] Enhancement
|
Nice! This looks really good:
Potentially we could merge this as an iteration, pending a rebase to fix the conflicts. Further improvements would be for these two pieces, which should probably be separate PRs we can consider:
The centered "Add background image" looks a little off, we might want to left align that text and add an upload icon in the spot on the left. And the background image inside the ItemGroup once uploaded should have a rounded-rectangle shape, not the circle "swatch" shape. |
|
@jasmussen Thanks for the review. I have rebased the branch. |
|
@jasmussen what do you think about an empty state just like colors?
|
|
These should have the same exact UI as the items in the Color panel:
The color panel:
|
|
Could use the same treatment as the Colors panel "Elements" perhaps (with no border divider too):
|
Love it. |
|
Thanks @richtabor & @jasmussen for your review. I have addressed the border styles on focus issue and also added the upload icon for background image and changed its shape. Can you please review it again? Screen.Recording.2024-08-01.at.6.22.19.PM.mov |
|
Nice work, getting very close now:
The border radius on the square here should be 2px:
Also there's some border changes happening which means if you insert a group, the background control is missing a top border:
Since this is touching some ItemGroup related pieces, it might be good with a code check from @WordPress/gutenberg-components too. But functionally, this is 90% there. Thank you for the PR! |
…here is only one child in media-replace-container
|
@jasmussen Thanks for the review. I have addressed it now. |
|
Behavior-wise, design-wise, this is solid now, nice work, thank you! Code-wise this needs a review. There's a bit more CSS than I expected, that looks specialized mainly to this context. I wonder if our components group (already pinged) has a better approach we can take here, or not. Otherwise, this is good to go from the UI perspective. |
There was a problem hiding this comment.
I left some notes around component usage. The ItemGroup one is not necessarily blocking for this PR, as I believe it may require a substantial refactor. I do recommend cleaning it up though, as it looks quite fragile and hacky.
I'll tag @ramonjd for a better technical review as he may have insights into the feasibility/priority of a refactor.
packages/block-editor/src/components/global-styles/background-panel.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/background-panel.js
Outdated
Show resolved
Hide resolved
I think this is an existing issue with the ItemGroup, so not something that need be fixed in this PR. |
|
Would appreciate dev help and reviews, this would be a nice one to land! And thanks for contributing! |
| @@ -147,6 +152,14 @@ | |||
| } | |||
| } | |||
|
|
|||
| .block-editor-global-styles-background-panel__color-tools-panel-item { | |||
|
|
|||
| button.components-button { | |||
There was a problem hiding this comment.
While style overrides are
Instead of using button.components-button (which is meant to be a private API of the components package), could we assign a custom classname to the button itself and use that to apply style overrides?
Also, do we need to use a Button component, if we're going to override its styles? Should we consider just use a vanilla button instead?
(These comments apply to all instances of style overrides for Button in this file)
| @@ -83,13 +83,18 @@ | |||
| } | |||
|
|
|||
| .block-editor-global-styles-background-panel__image-tools-panel-item { | |||
| padding: 0; | |||
| flex-grow: 1; | |||
| border: 0; | |||
| .components-dropdown { | |||
There was a problem hiding this comment.
I know it was not introduced by this PR, but — as also mentioned about .components-button — .components-* classnames re not meant to be used outside of the components package. We can use custom classnames instead.
There was a problem hiding this comment.
Should I make the change in this PR only, or a separate PR would be good for it?
There was a problem hiding this comment.
Happy for it to be a separate, follow-up PR.
The main changes that would be needed:
- understand why we need to set a
36pxheight on.components-dropdown. Ideally, we should use the default button height. If not, let's understand why, and at most use therenderToggleprop to pass differently sized button; - avoid overriding color and focus styles of
.components-button, and only use the component props to tweak those aspects (mainly thevariantprop) - passing a custom
classNameto the components instances, and then using that custom classname in the styles file;
| /> | ||
| ) } | ||
| </ToolsPanelItem> | ||
| </div> | ||
| { shouldShowBackgroundColorControls && | ||
| path === '/background' && ( |
There was a problem hiding this comment.
Excuses in case I'm missing the context, but why do we need to check the path ?
Even if we needed to, this inline check in the middle of the JSX output could be easily overlooked. If we need this check, I would probably extract it as a prop of the Screen itself
There was a problem hiding this comment.
The BackgroundColorControl is also visible in block-level global styles. For example, if we remove this check, it will also be visible in the Group block's global styles.
Can you please tell me exactly where you want this to be moved as a prop?
There was a problem hiding this comment.
This component shouldn't really know about global styles routing, even more so that it's defined in a different package.
Instead, it could expose a boolean prop (something like showBackgroundControl or any other name that makes sense). Is showing those controls an exception? Then the default value of the boolean prop should be false. Otherwise, it should be true.
Let's assume it's false by defatul — this means that we should set it to true only where we need to show it, ie. in the background screen via the <BackgroundPanel /> component (and not sure where else).
There was a problem hiding this comment.
I have tried to implement it, could you please check again?
There was a problem hiding this comment.
Yup, you did follow my advice above.
However, I'd like to delegate review & advice on this aspect to someone with more experience with this part of the codebase, since I get the feeling that what I suggested is not optimal either.
In general, I get the feeling that, if we want the background panel to be rendered in a few places (global styles root, single block screen in global styles, and potentially anywhere else in the sidebar), we should generalize its logic a bit more — or delegate it to the parent react component.
For example, I wonder if we should make tweaks to the useHasBackgroundPanel hook (or create a new hook), since:
- it only seems to care about global settings, even if it looks like this panel needs to be used in block level screens too
- it only references the background image, while we need to also check for the background color (the main object of this conversation)
cc @ramonjd since it looks like he's done a bunch of work in this area of the codebase
There was a problem hiding this comment.
Thanks for helping out here @ciampo
In general, I get the feeling that, if we want the background panel to be rendered in a few places (global styles root, single block screen in global styles, and potentially anywhere else in the sidebar), we should generalize its logic a bit more — or delegate it to the parent react component.
The latter is definitely something worth to think about.
The proposal is to only add the background color picker to the background panel on for root global styles.
I suspect it'd be easier to test, and easier to use if <BackgroundPanel /> were generalized and the image/color/etc components abstracted and rendered as children of the former.
I'm not suggested a complete refactor of this PR, but it does need more testing.
it only seems to care about global settings, even if it looks like this panel needs to be used in block level screens too
I think that's consistent with other use* functions in sibling style panels. So, if theme.json turns these settings off, they're off everywhere. Unless I'm missing something 🤔
it only references the background image, while we need to also check for the background color (the main object of this conversation)
There's also a function of the same name useHasBackgroundPanel in the color panel that we should be using.
I'm going to get a PR up to fix the naming clash, but yeah, in order to determine whether to show the background panel at all, it should take into account all the properties.
It might be easier to reason about if things were abstracted.
There was a problem hiding this comment.
I think that's consistent with other
use*functions in sibling style panels. So, if theme.json turns these settings off, they're off everywhere. Unless I'm missing something 🤔
That makes sense — and shows how little experience I have with this part of the codebase 😓
The proposal is to only add the background color picker to the background panel on for root global styles.
I suspect it'd be easier to test, and easier to use if
<BackgroundPanel />were generalized and the image/color/etc components abstracted and rendered as children of the former.It might be easier to reason about if things were abstracted.
I trust your instinct on what the best solution should be.
But I definitely get the impression that the current implementation needs some re-architecting to make sure it can work well regardless of where it's rendered — root global styles screen, block-level global styles screen, or anywhere else in the inspector sidebar
| tabs={ [ | ||
| hasSolidColors && { | ||
| key: 'background', | ||
| label: __( 'Background' ), | ||
| inheritedValue: backgroundColor, | ||
| setValue: setBackgroundColor, | ||
| userValue: userBackgroundColor, | ||
| }, | ||
| hasGradientColors && { | ||
| key: 'gradient', | ||
| label: __( 'Gradient' ), | ||
| inheritedValue: gradient, | ||
| setValue: setGradient, | ||
| userValue: userGradient, | ||
| isGradient: true, | ||
| }, | ||
| ].filter( Boolean ) } |
There was a problem hiding this comment.
Potentially premature optimisation, but as of now the tabs array is being re-creater every time the component re-renders.
There was a problem hiding this comment.
I agree with @mirka that the background panel (and probably the rest of the global style screens) are pushing the boundaries of how a few components are supposed to be used (ItemGroup, Dropdown, ToolsPanel), showing that those components have some limitations that could be improved.
Apart from that (and some inline comments), I personally find the dropdown that opens when clicking "Add background image" a bit awkward, since most dropdowns / popovers would open to the left of the sidebar
…background-panel-global-styles
|
Flaky tests detected in 98833df. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10615849363
|
| @@ -33,10 +35,20 @@ function RootMenu() { | |||
| const hasShadowPanel = true; // useHasShadowPanel( settings ); | |||
| const hasDimensionsPanel = useHasDimensionsPanel( settings ); | |||
| const hasLayoutPanel = hasDimensionsPanel; | |||
| const hasBackgroundPanel = useHasBackgroundPanel( settings ); | |||
There was a problem hiding this comment.
The background panel doesn't show at all for me at the moment. I think we should be using rawSettings here, not the return value of useSettingsForBlockElement()
| const hasBackgroundPanel = useHasBackgroundPanel( settings ); | |
| const hasBackgroundPanel = useHasBackgroundPanel( rawSettings ); |
|
|
||
| const decodeValue = ( rawValue ) => | ||
| getValueFromVariable( { settings }, '', rawValue ); | ||
| const shouldShowBackgroundColorControls = useHasBackgroundPanel( settings ); |
There was a problem hiding this comment.
Hmm, I think there's an unfortunate naming clash. Wouldn't we be more interested in the function of the same same from the color panel?
I think one or both of the functions names could be changes, e.g. useHasBackgroundImagePanel and useHasBackgroundColorPanel
I'll get a quick PR up for that 👍🏻
In the mean time we could import the right function and rename it, e.g., import { useHasBackgroundPanel as useHasBackgroundColorPanel } from './color-panel';
| /> | ||
| ) } | ||
| </ToolsPanelItem> | ||
| </div> | ||
| { shouldShowBackgroundColorControls && | ||
| path === '/background' && ( |
There was a problem hiding this comment.
Thanks for helping out here @ciampo
In general, I get the feeling that, if we want the background panel to be rendered in a few places (global styles root, single block screen in global styles, and potentially anywhere else in the sidebar), we should generalize its logic a bit more — or delegate it to the parent react component.
The latter is definitely something worth to think about.
The proposal is to only add the background color picker to the background panel on for root global styles.
I suspect it'd be easier to test, and easier to use if <BackgroundPanel /> were generalized and the image/color/etc components abstracted and rendered as children of the former.
I'm not suggested a complete refactor of this PR, but it does need more testing.
it only seems to care about global settings, even if it looks like this panel needs to be used in block level screens too
I think that's consistent with other use* functions in sibling style panels. So, if theme.json turns these settings off, they're off everywhere. Unless I'm missing something 🤔
it only references the background image, while we need to also check for the background color (the main object of this conversation)
There's also a function of the same name useHasBackgroundPanel in the color panel that we should be using.
I'm going to get a PR up to fix the naming clash, but yeah, in order to determine whether to show the background panel at all, it should take into account all the properties.
It might be easier to reason about if things were abstracted.
There was a problem hiding this comment.
Thanks for your continued efforts on this @amitraj2203
It's not a straight forward task, so I appreciate your perseverance. 🙇🏻
I want these types of changes to move forward - I agree 100% that grouping controls that affect the background makes sense.
I'm just not sold that we're doing things in the right order, that is, reorganizing the UX on top of a styles system that doesn't support it.
For example, it's really confusing that I can't see my gradient background color behind the image:
It's because we haven't yet fixed the background/ background-image clash.
See the item under Setting background-image overrides background gradient in the background follow up tasks.
In a perfect world, this styles clash would have been fixed for 6.7, but time/resources were constraints.
With it fixed, the new UI proposed here would be the perfect complement.
I think we should concentrate on addressing the gradient/background-image CSS issue — which, in my opinion, is a fairly large bug and completely unintuitive to both developers and users — before we expose its limitation to users more obviously in the UX.
That's just my 2c. 😄
Also, on a more trivial point, if we're going with the square placeholder icon, I still think we should iron out the differences between its dimensions and the uploaded image's icon: #63216 (comment)
| </Item> | ||
| ); | ||
|
|
||
| function ColorPanelDropdown( { |
There was a problem hiding this comment.
I've mentioned it before, but I think this file is getting too large and probably needs some composition, e.g.,
<BackgroundPanel>
<BackgroundImageControls />
<BackgroundColorControls />
</BackgroundPanel>
Well, both. The disorganization of controls is certainly an issue, but it's already been an issue across Cover, Group, and the new unified background tools. This is a large undertaking, and one of the most important aspects of this PR, or perhaps more specifically, of #63096, is to move the background tools out of "Layout" and into the global styles inspector as a top level item. There are some gotchas with this, but this organization is an important step that ideally we find a path towards. |
Thanks @jasmussen Definitely not disputing the need to reorganize the controls - I only wanted to communicate that the way the underlying
My vote would be to only move the background image tool as a first iteration - it might make integrating color later easier to implement and test. 🤷🏻 |
Is this something you can help with? Thanks for the clarification! |
I don't have much development bandwidth for 6.7 unfortunately, but I think @amitraj2203's work so far has been great to illustrate how the controls can work (thank you @amitraj2203). In my view, we could split this PR into the things it's trying to achieve:
@amitraj2203 let me know if this makes sense to you - before 6.7 Beta 1 I should get some time to help out with 1 and 2. |
|
Following up on this comment: #63216 (comment) I've done points 1 and 2 in this PR: If that's okay, I think it will make merging background color and image controls (from this PR) a little less complex, at least it'll be fewer lines of code 🤞🏻 Thank you! |
|
Good call @ramonjd |
What?
Closes: #63096
Why?
To improve the organisation for background color and images by creating a new Background Panel in global styles.
How?
This PR adds a
Backgroundsection which collates both options.Testing Instructions
Screenshots or screencast
Screen.Recording.2024-07-07.at.3.55.52.AM.mov