Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Block toolbar: fix error #20458

Merged
merged 1 commit into from Feb 27, 2020
Merged

Block toolbar: fix error #20458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/edit-site/src/components/editor/index.js
View file
Open in desktop
Expand Up @@ -64,6 +64,7 @@ function Editor( { settings: _settings } ) {
content={
<>
<Notices />
<Popover.Slot name="block-toolbar" />
<BlockEditor />
</>
}
Expand Down
1 change: 1 addition & 0 deletions packages/edit-widgets/src/components/layout/index.js
View file
Open in desktop
Expand Up @@ -38,6 +38,7 @@ function Layout( { blockEditorSettings } ) {
content={
<>
<Notices />
<Popover.Slot name="block-toolbar" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we want one slot per widget area here or a single slot for the whole page. It's not clear how we should reason about this particular slot, where to put it if you're building your own editor. (maybe needs some docs)

Also, do we want to expose it as a BlockToolbarSlot in @wordpress/block-editor package?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the thing... I was hoping that we could build it into WritingFlow or BlockList (together with WritingFlow or something) to avoid exposing it. But maybe there's no other way. I also had an earlier idea to unify it with the top toolbar, in which case exposing it maybe makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, got it. I'm fine if we wait more and see if we can consolidate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the widgets screen question?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know so much about the widgets. Did I currently add one slot for all areas? If that works, that seems fine? What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tabbing order might not be great with this solution but at least the toolbar is showing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I guess it might be better to have one per area then. Let's iterate on it. Worth noting that previously, it wasn't possible to tab to the toolbar at all because it was mounted inside WritingFlow.

<div
className="edit-widgets-layout__content"
tabIndex="-1"
Expand Down
1 change: 1 addition & 0 deletions storybook/stories/playground/index.js
View file
Open in desktop
Expand Up @@ -43,6 +43,7 @@ function App() {
<BlockInspector />
</div>
<div className="editor-styles-wrapper">
<Popover.Slot name="block-toolbar" />
<BlockEditorKeyboardShortcuts />
<WritingFlow>
<ObserveTyping>
Expand Down