Global Styles: Adds site background color controls component in global styles#66736
Closed
amitraj2203 wants to merge 4 commits intotrunkfrom
Closed
Global Styles: Adds site background color controls component in global styles#66736amitraj2203 wants to merge 4 commits intotrunkfrom
amitraj2203 wants to merge 4 commits intotrunkfrom
Conversation
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
|
Contributor
Author
|
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
Member
ramonjd
left a comment
ramonjd
left a comment
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.
| onChange={ setStyle } | ||
| settings={ settings } | ||
| defaultValues={ BACKGROUND_DEFAULT_VALUES } | ||
| showColorControl |
Member
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 }> |
Member
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, |
Member
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
| className: clsx( | ||
| 'block-editor-global-styles-background-panel__dropdown-toggle', | ||
| { | ||
| 'is-open': isOpen, |
Member
There was a problem hiding this comment.
Much nicer than the useEffect 👍🏻 Great stuff
| defaultControls = DEFAULT_CONTROLS, | ||
| defaultValues = {}, | ||
| headerLabel = __( 'Background image' ), | ||
| headerLabel = __( 'Elements' ), |
Member
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()
| defaultValues = {}, | ||
| headerLabel = __( 'Background image' ), | ||
| headerLabel = __( 'Elements' ), | ||
| showColorControl = false, |
| }; | ||
| }, [] ); | ||
|
|
||
| const shouldShowBackgroundColorControls = useHasBackgroundPanel( settings ); |
Member
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 😄
| Dropdown, | ||
| Placeholder, | ||
| Spinner, | ||
| ColorIndicator, |
Member
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.
| settings?.background?.backgroundPosition || | ||
| settings?.background?.backgroundRepeat ); | ||
|
|
||
| const [ isDropDownOpen, setIsDropDownOpen ] = useState( false ); |
…ground-color-component-in-global-styles
Contributor
|
Hi @amitraj2203, do you have the bandwidth to resolve the code conflicts so that we're ready to get feedback again? Thank you! |
Contributor
|
With WordPress 6.9 Beta 1 just around the corner and we can no longer ship enhancements, so I'd like to punt this to 7.0. |
t-hamano
moved this from 🔎 Needs Review
to 🦵 Punted to 7.0
in WordPress 6.9 Editor Tasks
Contributor
|
Since this PR has been inactive for over a year and has many code conflicts, it would probably be better to restart work on it using the latest trunk branch. For now, let me close this PR. |
github-project-automation
bot
moved this from 🦵 Punted to 7.0
to ✅ Done
in WordPress 6.9 Editor Tasks
Contributor
Author
|
Hi @t-hamano, I wanted to check whether anyone is currently working on this. If not, I can pick it up and complete it by the end of this week. Just wanted to confirm before I start working on it. Thanks, |
Contributor
|
@amitraj2203 Thanks for the reply. As far as I know, no one has worked on this issue yet. Please feel free to reopen this PR or submit a new one. Do whatever works best for you 👍 |
Contributor
Author
|
@t-hamano Sure thing, I will raise a new PR. Thanks again! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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