Global Styles: add padding when no tabs in the background panel #44044
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
t-hamano
added
Global Styles
|
Size Change: +56 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
t-hamano
force-pushed
the
fix/global-styles-no-gradient-background-padding
branch
2 times, most recently
from
9a064b3 to
9ca03d7
Compare
t-hamano
changed the title
|
While I was rechecking this PR, I noticed that in #41900 there is a lot of discussion regarding color panels. I have updated the issue description and approach to account for its content. I would appreciate your advice as to whether this approach is appropriate. |
|
Flaky tests detected in 9ca03d7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3911040207
|
|
Nice one. It seems like this one needs technical feedback moreso than design feedback, as we definitely need that padding. The question is more, where should that padding be added, in which case I'll refer Lena or Marco 🙏 |
mirka
reviewed
Jan 17, 2023
Comment on lines
91
to
95
| .edit-site-screen-background-color__control { | ||
| .block-editor-color-gradient-control__panel { | ||
| padding: $grid-unit-20; | ||
| } | ||
| } |
There was a problem hiding this comment.
One small suggestion on the implementation — it would be ideal if we didn't have to rely on an internal CSS classname (.block-editor-color-gradient-control__panel) of a block-editor component. I think we can determine whether ColorGradientControl will be tabbed or not from the logic available in ScreenBackgroundColor. Then, you could just apply padding to .edit-site-screen-background-color__control itself when ColorGradientControl is not tabbed. Does that make sense?
There was a problem hiding this comment.
I think we can determine whether ColorGradientControl will be tabbed or not from the logic available in ScreenBackgroundColor. Then, you could just apply padding to .edit-site-screen-background-color__control itself when ColorGradientControl is not tabbed.
Does this mean that when only either the normal or gradient background is active (no tabs), the ColorGradientControl is not given a class?
In other words, the code is like this:
<ColorGradientControl
className={
( ! hasBackgroundColor || ! hasGradientColor ) &&
'edit-site-screen-background-color__control'
}
/>Or is it a code like this?
<ColorGradientControl
className={ classNames(
'.edit-site-screen-background-color__control',
{
'has-tab': hasBackgroundColor && hasGradientColor,
}
) }
/>There was a problem hiding this comment.
Yes, I was thinking more like the latter. The CSS might be simpler if we flip the boolean though (.edit-site-screen-background-color__control.has-no-tabs { padding: $grid-unit-20; }). (No strong opinion on the actual naming!)
There was a problem hiding this comment.
I have refactored and confirmed that it works as expected as before 👍
Thanks for checking! In this case, no worries, we'll deal with that redesign later 😄 |
t-hamano
force-pushed
the
fix/global-styles-no-gradient-background-padding
branch
from
9ca03d7 to
4013317
Compare
t-hamano
force-pushed
the
fix/global-styles-no-gradient-background-padding
branch
from
4013317 to
3d209f1
Compare
mirka
approved these changes
Jan 18, 2023
Comment on lines
85
to
90
| .edit-site-screen-text-color__control, | ||
| .edit-site-screen-link-color__control, | ||
| .edit-site-screen-button-color__control { | ||
| .edit-site-screen-button-color__control, | ||
| .edit-site-screen-background-color__control.has-no-tabs { | ||
| padding: $grid-unit-20; | ||
| } |
There was a problem hiding this comment.
Nice, they're all in one place now 💛
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?
This PR adds
paddingwhen there are no tabs in the global style background panel.Currently, if a block supports only either a normal background color or a gradient color, tabs are not displayed, resulting in no padding on either side:
Why?
In the block color popover, this problem doesn't occur even when one of the background colors is not supported:
This is because padding is explicitly specified in the tab panel within the dropdown content:
gutenberg/packages/block-editor/src/components/colors-gradients/style.scss
Lines 49 to 51 in 2d44f06
I believe a similar approach should be taken for the global-style background panel.
How?
I used the wrapper
.edit-site-screen-background-color__controlas a selector to add padding.Testing Instructions
First, enable Empty Theme and update theme.json with the following definition. This definition changes the color panel as follows:
test/emptytheme/theme.json
Screenshots or screencast
04b31fb1ab2cabf61ae95e5520e72109.mp4