Unify the global block styles panel with the block inspector panels #49428
Conversation
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
jasmussen,
SaxonF,
noisysocks,
tellthemachines,
MaggieCabrera and
jameskoster
|
|
||
| const [ style ] = useGlobalStyle( prefix, name, 'user', false ); | ||
| const [ inheritedStyle, setStyle ] = useGlobalStyle( prefix, name, 'all', { | ||
| export default function DimensionsPanel() { |
There was a problem hiding this comment.
This is only rendered for the top level dimensions global styles screen, that's why I removed the arguments. Same for the other similar screens.
|
Size Change: -1.33 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Looks like a promising direction! It's a lot nicer to have all the controls immediately visible.
Figure out what to do with the color palettes per block (not visible in the current design)
Are per-block color palettes really a thing? Or is it just a view of the global palette? Feels a bit weird to define a color and have it only be available on that block 🤔
packages/edit-site/src/components/global-styles/screen-block.js
Outdated
Show resolved
Hide resolved
|
Before:
After:
I'm happy to approve this as a simplification of the structure. The ToolsPanel was always meant to curate the local view, while still surfacing every option in the global view. CC: @SaxonF as I know you've thought about this in the original issue. |
It is important for the "global view" because it means that palette will be available for all these blocks. Imagine a theme saying, buttons shouldn't use the global palette but have their own custom palette. It's not for just a specific button, it's for all the buttons of the site. |
I get that it's a global setting, but can't imagine a practical use-case for it. It would make sense for a certain block type, e.g. Button, to use only a subset of the theme palette, but a completely different palette would be pretty weird. |
youknowriad
force-pushed
the
update/block-screen
branch
from
7c6d793
to
17bf817
Compare
|
Flaky tests detected in 8acff4c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4891088640
|
|
Wishlist: please place the style variation names vertically, not horizontally two and two, because there are issues with the names being truncated. Let's say you have one blue and one black shadow; the truncated name would only be "Shadow: Bl..." |
|
Thanks again for working on this @youknowriad . Video below shows how we might handle variations (also being discussed here ) as well as colour palette's per block. Custom CSS drill down link can just be moved into advanced section at the bottom so that it's consistent with "Additional CSS Class(es)` in local view of block. The block palette screen copy should probably be customised to indicate its only changing for the particular block you're in. An added benefit of all this work is that a user could style a block up locally in the context of a post/page and then push globally and the experience would be the same. globalvslocal.mp4 |
Can you expand a bit on what you see as the role of the block-specific palette? I may be missing some details, and I'm also separately thinking ahead around how we can surface "refs" for colors, i.e. "for the button background use the default text color" or "for the button text use the global background color" linked colors. |
Just covering existing functionality per @youknowriad comment |
youknowriad
force-pushed
the
update/block-screen
branch
from
17bf817
to
22e096f
Compare
|
Ok, this should be ready now to get more testing and reviews. I've restored all the functionality we have in trunk, the only difference is that we're not able to tweak color palettes per block anymore. It seems folks are ok with that above? |
There was a problem hiding this comment.
This is a much, much nicer experience than trunk! The changes LGTM overall, just a few questions/comments below.
Do we want the controls inside the Dimensions panel to not display by default? I recall we were discussing that in #49389; if no one is opposed I'm happy to fix it so that they always display. It definitely makes sense in the site editor and I think it would be nicer in the post editor too.
Also, I noticed a "0" popping up under some of the block previews:
no idea where it's coming from, but it seems to be unique to this PR.
| blockType?.title | ||
| ) } | ||
| </p> | ||
| <StylesAdvancedPanel |
There was a problem hiding this comment.
This is not yet working for block style variations, but @carolinan has some work in progress in #49396 to address that. It should be fine to leave it as is for now.
It would be nice if the Advanced accordion stayed open after custom CSS has been added, to make it more visible. Otherwise it's easy to forget there's anything down there 😅
Do you think we can spin up a PR for this and invite designers for discussion there. Let's be consistent between panels. |
I just put up #50305. |
youknowriad
force-pushed
the
update/block-screen
branch
from
f985490
to
8acff4c
Compare
|
@youknowriad it looks like this PR broke the global styles specific behaviour for borders that we fixed in #49738 after the border panel refactor. The I haven't had the chance to get full context around this PR's changes yet to offer a proposed solution but I'm happy to review a fix though. |
|
Good catch @aaronrobertshaw I opened a fix in #50498 |
|
I understand the argument to unify panels, but as a user I prefer the current global block style panel because it makes it clear at a glance which properties can be modified at the top level (block dependent). |
| @@ -46,11 +39,11 @@ function ScreenColors( { name, variation = '' } ) { | |||
| ) } | |||
| /> | |||
|
|
|||
| <BlockPreviewPanel name={ name } variation={ variationClassName } /> | |||
| <BlockPreviewPanel /> | |||
There was a problem hiding this comment.
@youknowriad Is there a reason for rendering these BlockPreviewPanel instances that have no props? If the name prop is missing, there's no block to preview and the component is guaranteed to render null.
There was a problem hiding this comment.
I guess there's no reason since this PR actually removed the "block" specific versions of these screens. It was part of a refactoring so probably just an oversight.
closes #47348
What?
The Global Styles panel for blocks is very different than the block inspector for the same block. There's no clear reason for this difference. This PR brings them close together as per the designs in #47348
Todo:
Testing Instructions
1- Open the global styles for blocks (paragraph, image,...)
2- Make sure the design match the issue and work as expected.