Refactor <BlockToolbar /> #56335
Merged
Refactor <BlockToolbar /> #56335
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
|
Size Change: -136 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
jeryj
commented
Nov 21, 2023
jeryj
commented
Nov 21, 2023
youknowriad
reviewed
Nov 22, 2023
packages/block-editor/src/components/block-tools/block-toolbar-popover.js
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Nov 22, 2023
| @@ -192,7 +192,7 @@ export default function BlockTools( { | |||
| { /* Even if the toolbar is fixed, the block popover is still | |||
| needed for navigation and zoom-out mode. */ } | |||
| { ! showEmptyBlockSideInserter && hasSelectedBlock && ( | |||
| <SelectedBlockTools | |||
| <BlockToolbarPopover | |||
| __unstableContentRef={ __unstableContentRef } | |||
| clientId={ clientId } | |||
There was a problem hiding this comment.
Is clientId useless here?
There was a problem hiding this comment.
It is used within the component, but we could fetch it from within the <BlockToolbarPopover /> instead if you'd like. I had it passed it because it was already available, but if you prefer letting <BlockToolbarPopover /> and <EmptyBlockInserter /> handle them on their own, that's fine with me!
youknowriad
reviewed
Nov 22, 2023
youknowriad
reviewed
Nov 22, 2023
There was a problem hiding this comment.
Nice work here, it seems like a nice cleanup. You're saying the old component was not really usable unless you put it within NavigableToolbar, what happens now if this new component is inserted within NavigableToolbar (double NavigableToolbar) is that bad? is it acceptable compatibility? It seems fine to me. I also only expect third-party block editors to be using this component.
I would not use the current
I'd say these are all welcome enhancements. The biggest risk is stylistic changes from changed markup. Here's what the component looks like with a nested On a quick test, interactions appear to work fine and there are no console errors. It's just not styled well. |
|
Flaky tests detected in 58afab9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7132589444
|
|
The 'should hide the toolbar when typing' test is failing, but it's because it's checking for the toolbar to be |
This was referenced Nov 27, 2023
jeryj
force-pushed
the
refactor/block-toolbar
branch
from
6297c94
to
ede3596
Compare
jeryj
added
the
has dev note
|
@youknowriad Here's a direction for the block toolbar refactor. The only prop that is added to the BlockToolbar is |
jeryj
requested review from
kevin940726,
Mamaduka,
noisysocks,
draganescu,
talldan,
tellthemachines,
adamziel and
ellatrix
as code owners
There was a regression in the typing metric due to the BlockToolbar doing some processing on each keystroke.
jeryj
force-pushed
the
refactor/block-toolbar
branch
from
f3b5f4e
to
528665a
Compare
|
I believe I've addressed all feedback. Ready for another review! |
The naming was inconsistent and confusing. showBlockToolbar being true did not mean the block toolbar would show. Similarly for showBreadcrumb. Refactored so if showBlockToolbar is true, it will show the block toolbar popover.
ellatrix
approved these changes
Dec 7, 2023
This was referenced Dec 8, 2023
3 tasks
This was referenced Jan 10, 2024
Closed
11 tasks
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.
Addresses #56046
What?
<BlockToolbar />is not usable for the top toolbar in its current form as intended. This refactors and its implementation so it can be used directly in the top toolbar from #55787.Why?
The
<BlockToolbar />needs the<BlockContextualToolbar />wrapper to accomplish our usage within Gutenberg. If we make<BlockToolbar />useful on its own, then we can use it directly instead of relying on a private export of<BlockContextualToolbar />in the top toolbar headers.How?
<BlockContextualToolbar />into<BlockToolbar />. This brings all the necessary functionality for<BlockToolbar />to be used, mainly by wrapping it in a<NavigableToolbar />.<BlockContextualToolbar />. This is no longer needed, and was originally intended for Contextual to mean Popover.<BlockContextualToobar />in edit-site, edit-post, edit-widget, etc headers with<BlockToolbar /><SelectedBlockTools />becomes<BlockToolbarPopover />to better reflect its purpose.<PrivateBlockToolbar />with new props of__experimentalInitialIndex,__experimentalOnIndexChange, andfocusOnMount.<BlockToolbar />from<PrivateBlockToolbar />with locked__experimentalInitialIndex,__experimentalOnIndexChange, andfocusOnMountasundefined. Public API props arehideDragHandlesandvariantwith a default ofunstyled.isFixedfrom the<BlockToolbar />. The styles of the current small-screen fixed toolbar will become the default. The popover and top toolbars will have its own CSS overrides applied.<BlockToolbarPopover />and<BlockBreadcrumbPopover />into separate components for simplicity.<BlockToolbar />on your own."Follow-up
block-editor-contextual-block-toolbar? contextual was intended to mean "floating near the block," but contextual can also mean "context related to the selected blocks," so maybe it's better to leave it as is rather than break potential CSS implementations? Ideally, this would be theblock-editor-block-toolbarclass name and the nested div within it could be removed.<BlockToolbar />made me reconsider how to get the__experimentalInitialIndex,__experimentalOnIndexChange, andfocusOnMounthandled from<NavigableToolbar />. These are used by the block popover to focus the last focused index of the toolbar when using thealt + f10shortcut. And the reason they are needed is because the block toolbar unmounts onshouldShowContextualToolbar. But, if instead of unmounting it, we hide it with CSS, then we get all of the initial index and focusOnMount stuff for free since it's already handled by the roving tabindex. Those props can be removed from the code entirely. It also simplifies how the shortcut is handled.hideDragHandlein favor ofshowDragHandle = falseto default to not showing the drag handle since it's only used on the popover.Testing Instructions
trunkTesting Instructions for Keyboard
Dev Notes
<BlockToolbar />directly and it was wrapped in a<NavigableToolbar />component, remove the<NavigableToolbar />wrapper.<BlockTools />will not render any<BlockToolbar />. The<BlockToolbar />will needed to be added manually.