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

Block toolbar: fix error #20458

Merged
merged 1 commit into from Feb 27, 2020
Merged

Block toolbar: fix error #20458

merged 1 commit into from Feb 27, 2020

Conversation

@ellatrix
Copy link
Member

ellatrix commented Feb 26, 2020

Description

Fixes #20429.

This is a potential solution, but I'm not sure if it's the best.

The problem is that a popover slot may not always be provided, so the popover may not have a parent element with the popover-slot class.

Either we:

  • Add the popover-slot class to the root container. Not a very clean solution, but it work when the popover is rendered in place (not in a slot). We could change the class to something else if necessary.
  • Require the toolbar to render in the slot. (I don't think we should require this.)
  • Not use the popover slot div at all, but use a reference or class provided to Popover to determine what the boundary container is.

Perhaps the last solution is the best?

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.
@ellatrix ellatrix added the [Type] Bug label Feb 26, 2020
@ellatrix ellatrix requested a review from youknowriad Feb 26, 2020
@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Feb 26, 2020

Size Change: +26 B (0%)

Total Size: 865 kB

Filename Size Change
build/edit-site/index.js 4.63 kB +12 B (0%)
build/edit-widgets/index.js 4.42 kB +14 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 104 kB 0 B
build/block-editor/style-rtl.css 10.3 kB 0 B
build/block-editor/style.css 10.3 kB 0 B
build/block-library/editor-rtl.css 7.66 kB 0 B
build/block-library/editor.css 7.66 kB 0 B
build/block-library/index.js 116 kB 0 B
build/block-library/style-rtl.css 7.49 kB 0 B
build/block-library/style.css 7.5 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 90.9 kB 0 B
build/edit-post/style-rtl.css 8.59 kB 0 B
build/edit-post/style.css 8.58 kB 0 B
build/edit-site/style-rtl.css 2.51 kB 0 B
build/edit-site/style.css 2.51 kB 0 B
build/edit-widgets/style-rtl.css 2.59 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 4.01 kB 0 B
build/editor/style.css 4 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 879 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 26, 2020

Something I don't understand I do see <Popover.Slot /> used in both edit-widgets and edit-site though?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Feb 26, 2020

@youknowriad But not <Popover.Slot name="block-toolbar" />?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Feb 26, 2020

I think we might have to require it. Rendering the toolbar inline is not good since tabbing to it is broken then.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 26, 2020

Can we render it automatically at the beginning of <BlockList /> ?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Feb 26, 2020

That wouldn't work. It needs to be rendered outside of the writing flow. More specifically, outside the focus capture elements. It could be built into WritingFlow though. I do wish WritingFlow was built into BlockList...

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 26, 2020

I do wish WritingFlow was built into BlockList...

Me too :) I think the only thing prevent this is the PostTitle component in Gutenberg which in theory we could fix by duplicating some "arrow navigation handling in PostTitle" maybe?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Feb 26, 2020

I think ideally the post title needs to be some sort of real block. We can also not move the post title out of writing flow, otherwise reverse tabbing from the content will focus it. :) Either it needs to be a block, or we should provide a slot at the top of the block list to render the title in it from the outside?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Feb 26, 2020

I actually had already tried to move the popover slot to writing flow in #19431, but ran into some issues with reusable blocks I think.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 26, 2020

So maybe for now, let's just add the slot to the right position in the different screen/playground?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Feb 26, 2020

Yes, either that, or we explicitly pass the boundary container to the popover. I'll add the slots for now so it's at least no longer broken

@ellatrix ellatrix force-pushed the fix/block-toolbar-widget branch from b530efe to f2c961d Feb 26, 2020
@@ -38,6 +38,7 @@ function Layout( { blockEditorSettings } ) {
content={
<>
<Notices />
<Popover.Slot name="block-toolbar" />

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 26, 2020

Contributor

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?

This comment has been minimized.

Copy link
@ellatrix

ellatrix Feb 26, 2020

Author Member

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.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 26, 2020

Contributor

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 26, 2020

Contributor

What about the widgets screen question?

This comment has been minimized.

Copy link
@ellatrix

ellatrix Feb 26, 2020

Author Member

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?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 27, 2020

Contributor

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

This comment has been minimized.

Copy link
@ellatrix

ellatrix Feb 27, 2020

Author Member

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.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Feb 27, 2020

Thanks!

@ellatrix ellatrix merged commit cecd8ab into master Feb 27, 2020
3 checks passed
3 checks passed
build
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@ellatrix ellatrix deleted the fix/block-toolbar-widget branch Feb 27, 2020
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.