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

Experimental: allow parent Block to consume child Block's toolbar #18440

Open
wants to merge 35 commits into
base: master
from

Conversation

@getdave
Copy link
Contributor

getdave commented Nov 11, 2019

Several recent use cases have suggested that it would be valuable to a parent Block rendering child blocks into InnerBlocks to be able to "consume" capture the toolbars of the children (in reverse: the children have their toolbars "proxied" to the parent toolbar area to be displayed).

The key benefits of this are:

  1. the toolbar remains fixed to the top left avoiding the "toolbar overload" problem associated with having toolbars "follow" the cursor around as the child blocks are selected.
  2. the toolbar is rendered in a DOM node divorced from the child Block which works around difficult overflow: hidden problems.
  3. nested blocks are easier to use because the toolbar is always in a consistent position and not effected by the inner block layout.

Two specific use cases for this might be:

  1. Nav Block - see #18336
  2. Social Block

In general however, the pattern also lends itself well to any deeply nested Block hierarchies. For example, on the current master branch, try nesting a lot of Columns and see how you get on with nested toolbars. It isn't a ideal experience.

By deferring the rendering of all toolbars to the root block of the hierarchy we provide a better experience.

The PR is experimental and achieves the effect by:

  • Rendering a Slot for ChildToolbars in all parents of the currently selected Block.
  • Conditionally rendering the BlockContextualToolbar into the ChildToolbars Slot.

The reason for using Slot/Fill is due to needing to capture events triggered on the childtoolbar within the React component tree rather than the DOM tree. If we allow for the events to bubble in the DOM it triggers selection of the underlying parent Block (which is where the Slot is rendered) which breaks a lot of toolbar functionality.

This works. There are some quirks that would need to be worked out.

I'm looking for feedback about:

  1. Whether this goes too far in breaking the existing UI patterns (see Table Block which does something similar UI wise - although it doesn't use InnerBlocks or a Parent -> Child relationship)
  2. What a11y concerns there might be about having the child toolbar divorced from the block itself and could these be mitigated.
  3. Where my code is not considering edge cases.

How has this been tested?

Manual testing for now.

Testing

  1. Add Nav Menu
  2. Add some items
  3. Add some nested items
  4. Click between the Nav items and see all the child toolbars proxied to the top level Nav Menu.
  5. Test keyboard shortcut for jumping to closest toolbar works (ie: ⌥ + F10)

Also try switching out the boolean props on InnerBlocks on the Nav Menu Block and verify that the documented behaviour works as described.

Also try applying these props to the InnerBlocks of the Columns Block. Try nesting!

Screenshots

Screen Capture on 2019-11-15 at 14-52-08

Types of changes

New feature (non-breaking change which adds functionality)

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. .
@shaunandrews

This comment has been minimized.

Copy link

shaunandrews commented Nov 11, 2019

I'm a fan of this, as it keeps the tools in a consistent place and keeps things feeling a little simpler overall — less moving parts. Also avoids the awkward state where the toolbar can't move all the way and never really lines up with anything.

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Nov 11, 2019

I really like the idea of this as it has a toolbar that is relevant to what is happening.

@retrofox

This comment has been minimized.

Copy link
Contributor

retrofox commented Nov 11, 2019

Nice suggestion. It will affect how to deal with mobile.

@mtias

This comment has been minimized.

Copy link
Contributor

mtias commented Nov 11, 2019

This is discussed here, for reference: #17267

@getdave getdave requested a review from jasmussen Nov 12, 2019
@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Nov 12, 2019

I like the idea of this, I think it could work well where a parent block has a close relationship to its inner blocks, just like the nav and social links blocks.

Visually it might need some adjustment. I found two things, the left border seemed strange on the selected inner block now that it doesn't align with the toolbar. Secondly, I didn't think it was quite as clear that the inner block was selected. A bit of adjustment to those borders could probably solve both issues:
Screen Shot 2019-11-12 at 5 17 54 pm

It also seems like the more menu on inner blocks can't be selected. I realise the PR is probably pretty early in development, though.

From a code point of view, the terms consumeChildToolbar and proxyToolbarToParent seem like they could be simplified a bit for block implementors. Something like pinToolbarToParentBlock might be clearer.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 12, 2019

Nice work thank you!

This is what I see:

parent-toolbar

The shift in height on menu item select is a separate issue to this PR, but worth looking at.

I think this is good:

  • It scales better to a lot of contexts
  • As a prop, it is likely this will be helpful to a ton of exotic blocks we have yet to see — imagine a parent block where child blocks can be rotated or skewed, it would really help if the toolbar wasn't attached to those child blocks individually
  • Even though we are separately exploring ways to improve selecting nested blocks, this can further benefit specific blocks, at a block authors discretion, giving more tools to enable a good experience.

In that vein, 👍 👍

I would echo @talldan in suggesting that we might need some visual adjustment. There was something accidentally nice in having the toolbar centered over each menu item — that relationship would be nice to explore restoring. And also, the style of each selected child item can probably be holistically considered. But none of those explorations, IMO, should happen in this PR. What this does is essentially replace the hacky CSS that was written for the Navigation Block and Social Links blocks, to "mimic" this behavior in absence of this prop.

In that vein, it might be worth looking at how much of that code we can now remove and clean up, now that it's official. For example, #17877 might make the horizontal margin work simpler, and lines 14-68 in https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/social-links/editor.scss#L14 and 1-55 in https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/navigation-menu/editor.scss SHOULD be made redundant by this PR. I'm not they are, but they should be :)

@getdave getdave force-pushed the try/parent-consume-child-toolbar branch from ef671dd to eb2bca1 Nov 13, 2019
getdave added a commit that referenced this pull request Nov 13, 2019
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Nov 13, 2019

It also seems like the more menu on inner blocks can't be selected.

Agreed. Looks like hovering/clicking anywhere outside the format controls triggers the underlying parent Block to be selected.

Update

It's because the toolbar is rendered outside the selected Block. As a result, any clicks on items will be equivalent to clicking outside the Block. As the parent Block is "underneath" the child block then the result is that this parent Block becomes selected.

The reason why the bold/italic...etc buttons don't have this issue is because they are rendered via SlotFill and make use of the bubblesVirtually prop which causes events to...:

...bubble to their virtual parent in the React elements hierarchy

If you set the bubblesVirtually prop(s) to false here then these buttons also exhibit the same behaviour.

The issue, therefore, is working out how to stop these DOM events from bubbling.

Returning false in the onFocus handler, if the current Block is a parent of the selected Block, will fix this, but with the major drawback that you can then never actually focus the parent once a child is selected.

I'm starting to look into whether wrapping the Toolbar which omits the offending click event in <IgnoreNestedEvents /> can help here.

Question

I'm wondering whether to wait on the below to land before we try and spend too much time fixing the current implementation:

#17875

Copy link
Member

jorgefilipecosta left a comment

Hi @getdave thank you for submitting this proposal 👍 This mechanism is very useful in some blocks we have. I did an initial review and shared some comments.

@getdave getdave moved this from 👀 PRs to review to 💻 Issues in progress in Navigation block Nov 14, 2019
@getdave getdave self-assigned this Nov 14, 2019
@getdave getdave removed this from 💻 Issues in progress in Navigation block Nov 14, 2019
@getdave getdave force-pushed the try/parent-consume-child-toolbar branch from 77ba3bc to 6bcf14a Dec 2, 2019
@getdave getdave requested a review from nerrad as a code owner Dec 2, 2019
getdave added 2 commits Dec 2, 2019
As the API is experimental, remove it from any public Blocks for now.
Copy link
Member

jorgefilipecosta left a comment

This PR seems to work as expected 👍 I'm only noticing a small visual issue when applying the flag to the navigation block:
Untitled 5

Should we try to address it?

packages/block-editor/src/store/selectors.js Outdated Show resolved Hide resolved
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Dec 2, 2019

Should we try to address it?

Update: @jorgefilipecosta this looks to have been fixed in master and I've now rebased this PR.

Screen Shot 2019-12-04 at 11 07 32

@jorgefilipecosta I believe this to be a known issue unrelated to this PR. Not saying it shouldn't be addressed not sure this is the best place to do it just in case it is a larger problem. Let me see if I can track it down.

@getdave getdave requested a review from jorgefilipecosta Dec 3, 2019
Copy link
Member

jorgefilipecosta left a comment

Hi @getdave,
I'm sorry on the previous comment I shared the wrong screenshot.
The correct screenshot of the visual issue I'm facing is this one:
Untitled

When the toolbar moves to the parent the black line at the side should also move otherwise there is a visual disconnect.

@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Dec 13, 2019

@jorgefilipecosta I've been thinking more about this. I'm not totally convinced that moving the indicator is optimal for user experience.

For example, if I were to do this how would I know that I've hovered/selected the child block? The only indicator would be the block movers which I feel is insufficient.

Perhaps I can suggest we simply tweak the position of the focus/select indicator so it is placed in a nicer way. See below.

https://d.pr/i/Oq79cr/MUF2JwVfHi

(about to rebase this)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.