Global Styles: Adds site background color controls component in global styles #66736
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
amitraj2203
added
[Type] Enhancement
|
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: +706 B (+0.04%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in eb64f02. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11679183245
|
|
Hi @ramonjd, can you please review this PR and let me know if there is anything needs to be done. |
This was referenced Nov 20, 2024
ramonjd
reviewed
Nov 21, 2024
There was a problem hiding this comment.
Thanks for your work and patience on this @amitraj2203
It's coming along nicely.
I've added some comments. The TL;DR is:
- avoid using the color indicator for the background image preview
- ensure that theme.json settings work properly
I'll try to swing back to reviewing when I get a chance. Sorry that it's been so long.
| @@ -50,6 +50,7 @@ export default function BackgroundPanel() { | |||
| onChange={ setStyle } | |||
| settings={ settings } | |||
| defaultValues={ BACKGROUND_DEFAULT_VALUES } | |||
| showColorControl | |||
There was a problem hiding this comment.
What is the purpose of this prop? Shouldn't the theme settings control whether to show the color panel?
| }; | ||
|
|
||
| return ( | ||
| <Button __next40pxDefaultSize { ...toggleProps }> |
There was a problem hiding this comment.
The height added by __next40pxDefaultSize causes a mismatch in height, which is evident when the button has focus:
With __next40pxDefaultSize |
Without |
|---|---|
 |
 |
I know the linter wants us to have it there, but I don't see much value here.
Maybe it needs some overriding CSS to make it work.
Setting the height to height: 36px; appears to force it to match the other items.
| <ColorIndicator | ||
| aria-hidden | ||
| style={ { | ||
| '--image-url': imgUrl ? `url(${ imgUrl })` : undefined, |
There was a problem hiding this comment.
Is there any benefit to using a CSS var?
I'm not against it, just asking.
Maybe the CSS var could be namespaced, e.g., --wp-admin-background-image-preview-url
| 'block-editor-global-styles-background-panel__dropdown-toggle', | ||
| { | ||
| 'is-open': isOpen, |
There was a problem hiding this comment.
Much nicer than the useEffect 👍🏻 Great stuff
| @@ -96,7 +99,8 @@ export default function BackgroundImagePanel( { | |||
| panelId, | |||
| defaultControls = DEFAULT_CONTROLS, | |||
| defaultValues = {}, | |||
| headerLabel = __( 'Background image' ), | |||
| headerLabel = __( 'Elements' ), | |||
There was a problem hiding this comment.
These are not technically "elements".
I'm not sure what other term to use, possibly "Properties" or "Styles"...
For now we could change to "Options" or something.
Also, let's add headingLevel={ 3 } to the <ToolsPanel /> above in BackgroundToolsPanel()
| @@ -96,7 +99,8 @@ export default function BackgroundImagePanel( { | |||
| panelId, | |||
| defaultControls = DEFAULT_CONTROLS, | |||
| defaultValues = {}, | |||
| headerLabel = __( 'Background image' ), | |||
| headerLabel = __( 'Elements' ), | |||
| showColorControl = false, | |||
| }; | ||
| }, [] ); | ||
|
|
||
| const shouldShowBackgroundColorControls = useHasBackgroundPanel( settings ); |
There was a problem hiding this comment.
Hmm interesting. Should the color show if there's no background image? This test says "no" since useHasBackgroundPanel() checks for settings?.background?.backgroundImage.
However, if there's a background image, the color value will have zero effect, unless the user happens to be using a transparent PNG.
Also, if a theme turns off background colors in settings.color.background should that also have an effect here?
If the intention is to disable background colors then I think the answer is yes.
It's tricky. Overall, theme.json settings should really be in charge. For example:
"settings": {
"color": {
"background": false
},
"background": {
"backgroundImage": false,
}
}I think the path of least resistance is the following:
- If
settings.color.backgroundisfalse, don't show the color picker in the background image control. settings.background.backgroundImageisfalsedon't show the whole control.
For point 2, when settings.background.backgroundImage is false it'd be possible to show only the background color control and double up. I think that's a @jasmussen question 😄
| __experimentalHStack as HStack, | ||
| __experimentalTruncate as Truncate, | ||
| Dropdown, | ||
| Placeholder, | ||
| Spinner, | ||
| ColorIndicator, |
There was a problem hiding this comment.
I don't think we should use the color indicator component here. It's mainly customized for colors, and if something changes it might affect its usage here.
Also, there are some strange misshaped icons:
Kapture.2024-11-21.at.12.44.15.mp4
I think it'd be possible to roll our own icon here using the elements that exist, and also using the changes you've made here.
Kapture.2024-11-21.at.13.42.36.mp4
If it helps here's the diff from my local:
Diff
diff --git a/packages/block-editor/src/components/background-image-control/index.js b/packages/block-editor/src/components/background-image-control/index.js
index 7af1888d60..3f22d45f62 100644
--- a/packages/block-editor/src/components/background-image-control/index.js
+++ b/packages/block-editor/src/components/background-image-control/index.js
@@ -24,7 +24,6 @@ import {
Dropdown,
Placeholder,
Spinner,
- ColorIndicator,
__experimentalDropdownContentWrapper as DropdownContentWrapper,
} from '@wordpress/components';
import { __, _x, sprintf } from '@wordpress/i18n';
@@ -126,16 +125,22 @@ function InspectorImagePreviewItem( {
as="span"
className="block-editor-global-styles-background-panel__inspector-preview-inner"
>
- <ColorIndicator
+ <span
+ className="block-editor-global-styles-background-panel__inspector-image-indicator-wrapper"
aria-hidden
- style={ {
- '--image-url': imgUrl ? `url(${ imgUrl })` : undefined,
- } }
- className={ clsx(
- 'block-editor-global-styles-background-panel__inspector-image-indicator',
- { 'has-image': !! imgUrl }
- ) }
- />
+ >
+ <span
+ className={ clsx(
+ 'block-editor-global-styles-background-panel__inspector-image-indicator',
+ { 'has-image': !! imgUrl }
+ ) }
+ style={ {
+ '--wp-admin-background-image-preview-url': imgUrl
+ ? `url(${ imgUrl })`
+ : undefined,
+ } }
+ />
+ </span>
<FlexItem as="span" style={ imgUrl ? {} : { flexGrow: 1 } }>
<Truncate
numberOfLines={ 1 }
@@ -677,7 +682,7 @@ export default function BackgroundImagePanel( {
settings?.background?.backgroundRepeat );
return (
- <>
+ <div className="block-editor-global-styles-background-panel__inspector-media-replace-container">
{ shouldShowBackgroundImageControls ? (
<BackgroundControlsPanel
label={ title }
@@ -691,9 +696,7 @@ export default function BackgroundImagePanel( {
style={ value }
inheritedValue={ resolvedInheritedValue }
displayInPanel
- onResetImage={ () => {
- resetBackground();
- } }
+ onResetImage={ () => resetBackground() }
defaultValues={ defaultValues }
/>
<BackgroundSizeControls
@@ -710,11 +713,9 @@ export default function BackgroundImagePanel( {
style={ value }
inheritedValue={ resolvedInheritedValue }
defaultValues={ defaultValues }
- onResetImage={ () => {
- resetBackground();
- } }
+ onResetImage={ () => resetBackground() }
/>
) }
- </>
+ </div>
);
}
diff --git a/packages/block-editor/src/components/background-image-control/style.scss b/packages/block-editor/src/components/background-image-control/style.scss
index 971c877805..64dea516c7 100644
--- a/packages/block-editor/src/components/background-image-control/style.scss
+++ b/packages/block-editor/src/components/background-image-control/style.scss
@@ -1,13 +1,8 @@
.block-editor-global-styles-background-panel__inspector-media-replace-container {
- border: $border-width solid $gray-300;
border-radius: $radius-small;
// Full width. ToolsPanel lays out children in a grid.
grid-column: 1 / -1;
- &.is-open {
- background-color: $gray-100;
- }
-
.block-editor-global-styles-background-panel__image-tools-panel-item {
flex-grow: 1;
border: 0;
@@ -56,6 +51,10 @@
&:hover {
color: var(--wp-admin-theme-color);
}
+
+ &:focus {
+ box-shadow: inset 0 0 0 var(--wp-admin-border-width-focus) var(--wp-admin-theme-color);
+ }
}
.block-editor-global-styles-background-panel__loading {
@@ -76,21 +75,16 @@
height: 100%;
width: 100%;
padding-left: $grid-unit-15;
-
- &.is-open {
- background-color: $gray-100;
- }
-
- &:focus {
- box-shadow: 0 0 0 var(--wp-admin-border-width-focus) var(--wp-admin-theme-color);
- outline: none;
- }
}
.block-editor-global-styles-background-panel__dropdown-toggle {
cursor: pointer;
background: transparent;
border: none;
+
+ &.is-open {
+ background-color: $gray-100;
+ }
}
.block-editor-global-styles-background-panel__inspector-media-replace-title {
@@ -113,12 +107,33 @@
.block-editor-global-styles-background-panel__inspector-image-indicator {
background-size: cover;
border-radius: $radius-small;
-
+ width: 20px;
+ height: 20px;
+ display: block;
+ position: relative;
+ // Show a thin outline in Windows high contrast mode, otherwise the button is invisible.
+ border: 1px solid transparent;
&.has-image {
- background-image: var(--image-url);
+ background-image: var(--wp-admin-background-image-preview-url);
+ }
+ &.has-image::after {
+ background: unset;
}
}
+.block-editor-global-styles-background-panel__inspector-image-indicator::after {
+ content: "";
+ position: absolute;
+ top: -1px;
+ left: -1px;
+ bottom: -1px;
+ right: -1px;
+ border-radius: $radius-small;
+ box-shadow: inset 0 0 0 $border-width rgba(0, 0, 0, 0.2);
+ background: #fff linear-gradient(-45deg, #0000 48%, #ddd 0, #ddd 52%, #0000 0);
+ box-sizing: inherit;
+}
+
.block-editor-global-styles-background-panel__dropdown-content-wrapper {
min-width: 260px;
overflow-x: hidden;
@@ -138,6 +153,10 @@
.components-focal-point-picker::after {
content: none;
}
+
+ .block-editor-global-styles-background-panel__image-tools-panel-item {
+ padding: 0;
+ }
}
// Push control panel into the background when the media modal is open.
@@ -145,12 +164,6 @@
z-index: z-index(".block-editor-global-styles-background-panel__popover");
}
-.block-editor-global-styles-background-panel__popover {
- .block-editor-global-styles-background-panel__image-tools-panel-item {
- padding: 0;
- }
-}
-
.block-editor-global-styles-background-panel__media-replace-popover {
.components-popover__content {
// width of block-editor-global-styles-background-panel__dropdown-content-wrapper minus padding.
| @@ -683,23 +676,13 @@ export default function BackgroundImagePanel( { | |||
| settings?.background?.backgroundPosition || | |||
| settings?.background?.backgroundRepeat ); | |||
|
|
|||
| const [ isDropDownOpen, setIsDropDownOpen ] = useState( false ); | |||
…ground-color-component-in-global-styles
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.
What?
Builds upon #63216
Why?
As mentioned in #63216 (comment) and to organise the component structure of the background-color control in global styles.
How?
Testing Instructions
Screenshots or screencast
Screen.Recording.2024-11-05.at.12.53.29.PM.mov