Cover Block: Normalize the block toolbar #29247
Conversation
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
ajitbohra,
cameronvoell,
ellatrix and
guarani
as code owners
|
Size Change: +1.26 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
This PR needs mobile review as well as it impacts there too. |
|
Awesome, Riad. This is going to clean things up so much. Before:
After:
As you notice, yes, there's something off about the spacing. I'll see if I can push a fix after lunch. |
|
Also, "Add Media" should be "Add media" no? I.e. sentence case like menu items?
Or is there some proper noun rule I'm missing (noticing Reusable block with capital letter here)? Probably something I should look at separately 😅 |
|
It seems the capitalize nouns / sentence case goes back and forth in circles every couple years :) |
|
I think there's some kind of exception for blocks where it's |
|
The CSS that sizes sequential buttons has always been a little messy, due to fragments and differing types of nesting. I got close to getting it to work, by building upon the existing bundle of rules, but it started to break apart with plugins, and I experimented with a little refactor (which would address #24898). And I had something:
Yellow is first in a segment, orange is middle of a segment, red is last in a segment, green is a segment with just one control inside However that falls apart because of the accumulation of different nesting:
🙈 However, the heuristic COULD be simple: every segment is treated as if it has only a single control inside, unless a CSS class is present. The bold/italic/link inline controls use Otherwise, I'll dive into the CSS mess again :) |
|
So it seems when a plugin is active, which adds controls, groups are named "components-toolbar", when the same plugin is inactive, it's called "components-toolbar-group" 🙈 |
|
I personally think the idea solution is that the segments should be |
|
Agreed. I think I found a good interim solution, though, just putting the final touches on it. |
|
Alright, that was a massive headache, but in 3d7a7a7 I managed to refactor the CSS to be what I believe is both easier to read, and more resilient. But because it touches sensitive code, it really means we need to test this a lot for every block toolbar that ships, and ideally with a few plugins as well. I tested with coblocks and it works solidly.
The thing is, both CSS classes and markup of the block toolbar change when a plugin adds buttons to the block toolbar. It's quite unwieldy. There are some stray But we're in a better place with this, so that's the good news. Fixes #24898. |
|
|
Oh thank you, that's very helpful context, makes sense. The reason it makes it gnarly is that components-toolbar is used at multiple levels of nesting, so it's very hard to create rules that are simple and readable yet reach their target consistently for this CSS. Can the legacy rule be renamed? And/or can it coexist (i.e. To be clear, for this PR I believe I solved the CSS. But for the longer term, I would really love for the block toolbar structure to be simplified and more predictable — groups and either buttons or dropdowns, as Riad suggests, so 2 levels deep at most. |
|
If we don't need to maintain backward compatibility for class names, I'd say we should try to get rid of Also, there's another inconsistency where some toolbar buttons will have a wrapping div ( gutenberg/packages/components/src/toolbar-button/index.js Lines 32 to 61 in 06f741e That's why we have this kind of rule where we're expecting a wrapping div: gutenberg/packages/components/src/toolbar-group/style.scss Lines 70 to 71 in 06f741e Ideally, I think we should get rid of both the |
youknowriad
force-pushed
the
update/normalize-cover-block-toolbar
branch
from
3d7a7a7
to
450f03b
Compare
youknowriad
force-pushed
the
update/normalize-cover-block-toolbar
branch
from
f3e4985
to
f1853fb
Compare
youknowriad
removed
the
Needs Dev Note
Related #25983 similar to #29205
This PR normalizes the Cover blocks toolbar like suggested in the linked issue. That said, unlike the image block, the cover blocks needs to merge controls coming from several places in the code base into the same segment:
In order to support adding controls to the same segment from multiple places, I added a
segmentprop to theBlockControlscomponent (which means multiple slots, one per segment). The other difference is that these segments are automatically wrapped inToolbarGroup.I tried to keep this backward compatible: still supporting the default segment without any toolbar group wrapper but if this is a validated direction, we might want to update the documentation (design and dev) about these "segments" and encourage folks to use them.
Notes