Block editor: Storybook: BlockTools must wrap around editor #56545
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
ellatrix
added
[Package] Block editor
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
Size Change: 0 B Total Size: 1.7 MB ℹ️ View Unchanged
|
youknowriad
reviewed
Nov 27, 2023
| > | ||
| <BlockTools /> | ||
| <BlockCanvas height="100%" styles={ editorStyles } /> | ||
| <BlockTools> |
There was a problem hiding this comment.
Why block tools need to wrap BlockCanvas? why shouldn't we just embed it in BlockCanvas? (Goal is to have the least possible code to write for developers).
Also I'm assuming that this change is going to make the toolbar "contextual" (by the block)? right? how can a consumer render the toolbar in its desired position (like a fixed toolbar in the post editor) or elsewhere?
There was a problem hiding this comment.
BTW, the "box" playground is meant to create something like "TinyMCE" where the toolbar is fixed at the top (something like the isolated block editor experiment). So this change is actually undoing that.
There was a problem hiding this comment.
Ok It seems the linked PR is what broke these stories. I commented on it. #55787 (comment)
There was a problem hiding this comment.
Are you sure it was working before that PR?
There was a problem hiding this comment.
@ellatrix yes, I did create these two stories myself when I was writing the platform-docs to confirm how third-party developers should do it.
There was a problem hiding this comment.
I tried using the top toolbar setting, and BlockTools still renders a toolbar if the screen size is not "large".
This was intentional to match the existing behavior. The block toolbar is supposed to sit underneath the header at large screens for either popover or top toolbar settings.
There was a problem hiding this comment.
This was intentional to match the existing behavior. The block toolbar is supposed to sit underneath the header at large screens for either popover or top toolbar settings.
In some third-party editors, there might not be any header. So when we touch these APIs, we should consider these use-cases. Is this behavior something that should be baked in or something that's left to the consumer.
In this case, there's no "Header" component exposed by block-editor, so probably not something we can bake it in a generic way.
There was a problem hiding this comment.
What are the available presentation options for <BlockToolbar /> in <BlockTools />? My understanding was popover until smaller screen, which gets fixed to the top of the editor pane. Instead, should it be <BlockTools blockToolbar="popover|fixed|none" /> and we pass the option we want into <BlockTools />?
Then, for our editor, the flow looks like:
// begin pseudo code
const isTopToolbar = getSettings().hasFixedToolbar;
const isLargeViewport = useViewportMatch( 'medium' );
let blockToolbarDisplay = 'popover';
if ( isLargeViewport ) {
// pinned to top of screen
blockToolbarDisplay = "fixed";
} else if ( isTopToolbar ) {
// Handled by the external header. BlockToolbar is never rendered within `<BlockTools />`
blockToolbarDisplay = "none";
}
<BlockTools blockToolbar={ blockToolbarDisplay } />
There was a problem hiding this comment.
Let's not use BlockTools please, it's also needed for shortcuts and inline inserter. BlockTools should be absorbed into BlockCanvas and we should export another component to create a top toolbar.
There was a problem hiding this comment.
Oh yeah, no intention of exporting <BlockTools /> for the top toolbar. My code example was to remove the concept of a top toolbar from <BlockTools /> due to @youknowriad's explanation:
If hasFixedToolbar is false (default), the BlockContextualToolbar is rendered, otherwise it's not and you are (consumer of block-editor package, in our case edit-* packages) responsible of rendering the block toolbar by using the component directly.
#55787 (comment)
<BlockToolbar /> is the component we're trying to get to be easily useful for the top toolbar. Right now we had to privately export <ContextualBlockTools />, so #56335 is trying to make <BlockToolbar /> useful on its own instead of relying on <ContextualBlockTools />.
youknowriad
reviewed
Nov 27, 2023
| </div> | ||
| <BlockCanvas height="100%" styles={ editorStyles } /> | ||
| <BlockTools> |
There was a problem hiding this comment.
Yeah the goal of this story was to show that we can show the block toolbar after the "undo/redo" buttons in the same kind of toolbar.
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?
There's currently two instances in Storybook that don't correctly use
BlockTools. It must wrap around the editor for it to work, otherwise you get an error.Additionally I removed
hasFixedToolbarbecause that doesn't work. Maybe in the future we need to expose a Slot or Component that you can render elsewhere. There's some context in #55787.Also, the positioning of the toolbar is off, we'll need to figure out what is wrong in CSS. Most likely we also need to pass the content ref to the block tools, which is something we should build in (maybe with context).
From using the components elsewhere, I can tell it's quite tricky to get the floating toolbar to work nicely btw, you need to ensure there's enough padding around the content so that it doesn't overlap with content. For some reason the logic where it should move it under the block is broken.
Why?
How?
Testing Instructions
Open the Storybook block editor section. Type in a block and move the mouse so the block toolbar shows (it doesn't show with an empty paragraph).
Testing Instructions for Keyboard
Screenshots or screencast