Stacked/unified block toolbar #31134
Conversation
|
Size Change: -1.02 kB (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
try/stacked-unified-block-toolbar
branch
from
bb43d82
to
1f190db
Compare
ellatrix
requested review from
adamziel,
draganescu,
kevin940726,
noisysocks,
talldan and
tellthemachines
as code owners
| @@ -34,7 +35,7 @@ function MyEditorComponent() { | |||
| onChange={ ( blocks ) => updateBlocks( blocks ) } | |||
| > | |||
| <SlotFillProvider> | |||
| <Popover.Slot name="block-toolbar" /> | |||
There was a problem hiding this comment.
@youknowriad I think you'll be happy to see this 😄
|
|
||
| return ( | ||
| <div className="block-editor-block-contextual-toolbar-wrapper"> |
There was a problem hiding this comment.
This class is not used anywhere, so it should be safe to remove this useless div.
| @@ -512,6 +512,25 @@ | |||
| border-right-color: $gray-900; | |||
| } | |||
|
|
|||
| &.is-fixed { | |||
| position: sticky; | |||
There was a problem hiding this comment.
I feel the is-fixed styles and logic are misplaced, they make more sense in the edit-* packages because we don't really know the container here and these styles depend on their container right?
There was a problem hiding this comment.
Sure, we could move it. I don't mind either way. sticky just falls back to being relative if there's no scroll container though.
There was a problem hiding this comment.
For example, in the site editor there's no scroll container, but the content box below is scrollable.
ellatrix
force-pushed
the
try/stacked-unified-block-toolbar
branch
from
27a0b7e
to
e7dbc43
Compare
|
I love this so much! 👏🏻 |
|
Thanks so much for working on this, it's a big one! Mostly I love the huge amount of red code in this one. Let me dive in. Here's the post editor before:
Here's after:
Here's the site editor before:
Here's the site editor after:
I'm seeing one issue:
Note how there's always a little extra scrollspace at the bottom. I'm also seeing this in the site editor, but it's less pronounced there. Let me know if you need help debugging this. Let's get back to the "Why" of this PR, especially if you're used to seeing this:
Because it isn't immediately obvious why this is better:
The empty toolbar also takes up space even when you have unselected blocks:
Those are good questions, and they are opportunities to improve the overall design of the top toolbar. There's in fact a dedicated issue for it, #20592, wherein I think we should explore those solutions. However here's why I think it's still important to land this PR today. Firstly, the toolbar is entirely broken in the site editor:
Even when the toolbar arbitrarily stacks, it covers content:
The tabbing order is also broken as soon as stacked:
This PR fixes all that:
For that reason, I'm a fan, and I think we can follow up on improving the visuals even further as part of #20592. |
|
It makes sense both visually, for accessibility and in the code. I'll look into the issues in a bit. |
|
One other thing, in the post editor, the scrollbar starts at the top:
In the site editor, it starts under the white bar. The latter is ideal, the former less so. But I presume that if/when the iframe could land for the post editor, we'd get the latter there as well? |
|
@jasmussen What do you mean, do you not expect it to start at the top? There's no way to avoid that with sticky positioning. That's just how sticky works. |
ellatrix
force-pushed
the
try/stacked-unified-block-toolbar
branch
from
b98040a
to
0c4b6d2
Compare
| [] | ||
| ); | ||
|
|
||
| if ( hasFixedToolbar ) { |
There was a problem hiding this comment.
Testing this PR and on mobile viewports (with Top Toolbar disabled), there's no block toolbar.
My understanding is that the desired behavior is for the fixed toolbar to show on mobile viewports.
I think there are still a few files that have some viewport logic for hiding the floating toolbar on mobile viewports (BlockPopover, BlockToolbar).
I wonder if as much of that viewport logic could be consolidated into one place, possibly this component and this if statement.
One of the difficult parts of implementing an editor is that the mobile version of the block toolbar doesn't work out of the box, a developer has to specifically add the fixed toolbar even if they're not implementing top toolbar mode, so it'd be nice if UnifiedBlockToolbar helped solve that.
At the same time, if an implementor wants the responsiveness of the toolbar to behave differently to the post editor, that's difficult to do as the viewport logic is implicit to much of the block editor package. I think ideally BlockToolbar would be just a plain configurable toolbar that we build abstractions around.
|
@tellthemachines I have to check trunk, but the toolbar was previously fixed too, so I preserved the styling. The fixed toolbar is also not enabled there (no option and no small viewport), so I don't think sticky positioning is enabled at all. |
ellatrix
force-pushed
the
try/stacked-unified-block-toolbar
branch
2 times, most recently
from
15a5cdf
to
df4f39a
Compare
|
In trunk, the toolbar in the customizer isn't showing at all on mobile, so this PR is already an improvement on that 😄 |
ellatrix
force-pushed
the
try/stacked-unified-block-toolbar
branch
from
df4f39a
to
bfee573
Compare
| return ( | ||
| <InsertionPoint> | ||
| { ( hasFixedToolbar || ! isLargeViewport ) && ( |
There was a problem hiding this comment.
In an earlier iteration this component was just implementing the two forms of the block toolbar, and I think it was good to include the viewport logic in that iteration. The UnifiedBlockToolbar component became a convenient and quick way to implement that kind of responsive toolbar.
But now this is bundled with some pretty key functionality for the block editor it seems fairly opinionated to say that someone implementing an editor should have a fixed toolbar on small viewports.
I don't think it's a blocker, this PR still makes things much easier overall. And an implementer can still reimplement this component without the viewport code. But I think it does pose a question about whether BlockEditor should be exporting high-level or low-level components.
There was a problem hiding this comment.
We can have a prop on this component hasFixedToolbar that overrides the setting? That's easy to add. :)
There was a problem hiding this comment.
No rush to do that right now, just some food for thought. 😄
ellatrix
force-pushed
the
try/stacked-unified-block-toolbar
branch
from
e2c2fbe
to
1de2eb8
Compare
There was a problem hiding this comment.
This is working really well, great job @ellatrix 🎉
I tested back compat by reverting the navigation editor's files back to trunk and it works perfectly.
The only issue I spotted is this behaviour when there are notices with the Top Toolbar or Fixed Toolbar, which would be good to tackle in a follow up:
I'll go ahead and merge and follow up myself with any needed adjustments for the widgets screens.
| display: flex; | ||
| flex-flow: column; |
There was a problem hiding this comment.
It doesn't seem to cause any issues, but I couldn't quite see why this change was made.
There was a problem hiding this comment.
flex-flow: column; is a magical way to stack elements with the total height remaining 100%. If one of the elements sets its height to 100%, it will just fill up the remaining height. Cc @jasmussen
There was a problem hiding this comment.
Very cool indeed.
I'll keep an eye on any flex related issues that might occur from this, but in my testing this worked as intended, and it lets us avoid doing something like height: calc(100% - #{ $block-toolbar-height } ) for the canvas.
| return ( | ||
| <InsertionPoint> | ||
| { ( hasFixedToolbar || ! isLargeViewport ) && ( |
There was a problem hiding this comment.
No rush to do that right now, just some food for thought. 😄
talldan
added
[Package] Block editor
|
💯 |
|
Thanks for the reviews! |
Description
Best reviewed without whitespace changes.
Alternative to #30234.
Also fixes #26114, a long standing accessibility issue with the inline rich text toolbar.
Also unifies toolbar APIs under one simple
<BlockTools />component and removes the need for implementors to render a dedicated block toolbar popover slot.How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).