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

Add appender to Block Navigator #18100

Merged
merged 5 commits into from Nov 6, 2019
Merged

Add appender to Block Navigator #18100

merged 5 commits into from Nov 6, 2019

Conversation

@talldan
Copy link
Contributor

talldan commented Oct 25, 2019

Description

Closes #17543.

Adds an appender to blocks that have inner blocks in the block navigator.

How has this been tested?

  1. Enable the experimental navigation block
  2. Add a navigation block to a post
  3. Make sure the block has some menu items
  4. Open the block navigator from the block toolbar

Screenshots

Screen Shot 2019-10-25 at 3 03 34 pm

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.
} ) {
const shouldShowAppender = showAppender && !! parentBlockClientId;

This comment has been minimized.

Copy link
@talldan

talldan Oct 25, 2019

Author Contributor

I had some ideas about improving the logic in this component around showing the appender. At the moment it shows the appender for any block that has childBlocks. An improvement might be:

  • Show the appender if the block type supports it, e.g. supports: { __experimentalBlockNavigatorAppender: true }
  • Show the appender if the block type supports only a single type of inner block (so that the block is added immediately).

This comment has been minimized.

Copy link
@mtias

mtias Oct 25, 2019

Contributor

Showing it only if it has child blocks (which is the equivalent of showing it only at the end of inner block areas) seems the correct way to me. The fact it adds a block immediately (the parent only supports a single child) or opens a full inserter should be irrelevant for the navigator.

This comment has been minimized.

Copy link
@talldan

talldan Oct 25, 2019

Author Contributor

Ok, so happy with a popover in a modal?

I can look into getting that working. Cheers for the quick feedback. 👍

This comment has been minimized.

Copy link
@mtias

mtias Oct 25, 2019

Contributor

Yes, though since we are going to enable this only within navigation menu initially, which only supports a single child, it should affect things much.

@talldan talldan force-pushed the add/block-navigator-appender branch from 815486b to b090164 Oct 30, 2019
@talldan talldan changed the title Blocked: Add appender to Block Navigator Add appender to Block Navigator Oct 30, 2019
@talldan talldan mentioned this pull request Oct 30, 2019
5 of 5 tasks complete
@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Oct 30, 2019

I haven't attempted to get the inserter menu working in the Block Navigator, but now #16708 is merged this is working a bit better and it inserts menu items without showing the menu. I think it's good for a design/code review.

Some fixes to insertion present on this PR are also on a separate PR #18178 that I expect will be merged first.

@talldan talldan force-pushed the add/block-navigator-appender branch from 0fce7cc to 4959615 Oct 31, 2019
@noisysocks noisysocks added this to 👀 PRs to review in Navigation block via automation Oct 31, 2019
Copy link
Member

noisysocks left a comment

This works great—nice job! 👍

inserting

Two smallish things:

  1. When there are no menu items, the button doesn't appear:
    Screen Shot 2019-10-31 at 14 43 10

  2. When testing with VoiceOver, there's no feedback that a block has been inserted when you select the append button. An wp.a11y.speak() could be good here.

@@ -43,8 +44,14 @@ export default function BlockNavigationList( {
blocks,
selectedBlockClientId,
selectBlock,
showAppender,

// Internal use only.

This comment has been minimized.

Copy link
@noisysocks

noisysocks Oct 31, 2019

Member

This is clever and made me think about how I'd love to see props grouped into // Public and //Private for all our components. I'm always finding it tough to figure out whether a prop is public or e.g. added via withSelect.

(Just a thought—nothing to do with this PR!)

@@ -15,12 +15,13 @@ import { _x, sprintf } from '@wordpress/i18n';
import BlockDropZone from '../block-drop-zone';
import Inserter from '../inserter';

function ButtonBlockAppender( { rootClientId, className } ) {
function ButtonBlockAppender( { rootClientId, className, __experimentalSelectBlockOnInsert: selectBlockOnInsert } ) {

This comment has been minimized.

Copy link
@noisysocks

noisysocks Oct 31, 2019

Member

The argument that dispatch( 'core/block-editor' ).insertBlock accepts is named updateSelection. Should we re-use that name for consistency?

This comment has been minimized.

Copy link
@talldan

talldan Oct 31, 2019

Author Contributor

<ButtonBlockAppender updateSelection={ false } /> didn't seem as clear to me as <ButtonBlockAppender selectBlockOnInsert={ false } />.

@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Oct 31, 2019

When there are no menu items, the button doesn't appear

Yeah, this is tricky, because we don't want it to appear for every block that can have inner blocks. For example it'd look incredibly busy if every Menu Item had an appender nested under it. So this is the trade-off I made to make it work dynamically, but still look uncluttered. I think it's ok for MVP, particularly as there's less need to use the Navigator when there's no menu items.

In an earlier comment I mentioned using a supports property - #18100 (comment). This is basically the reason—the Navigation Menu could then be set up to always show the appender while Menu Items don't. Alternatively, perhaps the design needs to be tweaked so the layout is less cluttered while appenders are still present for every block that can have inner blocks.

When testing with VoiceOver, there's no feedback that a block has been inserted when you select the append button. An wp.a11y.speak() could be good here.

Good catch. I suppose since the block isn't selected there's no indication to a screenreader. I've added something to fix that now.

@talldan talldan force-pushed the add/block-navigator-appender branch from df43d00 to c4bde22 Oct 31, 2019
@mtias

This comment has been minimized.

Copy link
Contributor

mtias commented Oct 31, 2019

Yeah, this is tricky, because we don't want it to appear for every block that can have inner blocks.

I've been thinking about an idea where we attempt to render some or all of the parents inspector controls in a child when the child has a parent: blockType configuration. It could be useful for things like the customization panel, so you can change the color of all menu items from within a single menu item. The way I'd imagine it could work is related to #18018 and having something like BlockInspectorGroup makeAvailableForChildren={ true }. The UI would have to be clear in separating a bit the controls that are for the child and the ones that come from the parent. It could also be a basis or related to #1224 in terms of declaration. cc @gziolo @youknowriad for thoughts.

It's a bit of a tangent because we could treat the nested structure as a special case.

@talldan talldan force-pushed the add/block-navigator-appender branch from c4bde22 to 85eaa1f Nov 4, 2019
@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Nov 4, 2019

In a final round of testing I've spotted a bit of an issue.

If you go into the navigator without having selected a menu item beforehand, the allowedBlocks restrictions don't work. In this screenshot, only 'Menu Item' should be allowed:
Screen Shot 2019-11-04 at 2 59 36 pm

The issue is that because the menu item's InnerBlocks component hasn't been rendered, the allowedBlocks it defines never has a chance to be registered in the store:

{ ( isSelected || isParentOfSelectedBlock ) &&
<InnerBlocks
allowedBlocks={ [ 'core/navigation-menu-item' ] }
renderAppender={ hasDescendants ? InnerBlocks.ButtonBlockAppender : false }
/>
}

Currently the code that checks for allowed blocks treats this as an absence of the prop, so considers that any block is valid.

A fix for the navigation menu might be to rework the ( isSelected || isParentOfSelectedBlock ) logic that prevents the rendering of InnerBlocks. However, there's still an issue that third party blocks might also choose to use logic like this, and the appender just won't work correctly in that case.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 4, 2019

I've been thinking about an idea where we attempt to render some or all of the parents inspector controls in a child when the child has a parent: blockType configuration. It could be useful for things like the customization panel, so you can change the color of all menu items from within a single menu item. The way I'd imagine it could work is related to #18018 and having something like BlockInspectorGroup makeAvailableForChildren={ true }. The UI would have to be clear in separating a bit the controls that are for the child and the ones that come from the parent. It could also be a basis or related to #1224 in terms of declaration. cc @gziolo @youknowriad for thoughts.

I had some similar thoughts on a bit different use cases but overall I think it falls under the same group of UI enhancements offering a fluid way of changing the post in the given context. My use case was the Group and Paragraph blocks. In particular, I was thinking that we could provide a set of mechanics that allows you to have bare Paragraph block as long as it's unstyled. At the same time, Group block as a default parent handler (and implicit parent block) would expose its controls to the Paragraph block. This way, you would no longer need to duplicate tons of attributes and its handlers on each leaf block but you could render some of the controls from the parent based on that relationship. On the technical level, it would wrap the existing block with the parent block behind the scenes when such a shared attribute would be set.

To your point, I think that's both technically possible with the block edit context and also highly useful for the end-users. I'd love to see us exploring it.

@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Nov 4, 2019

@talldan in Firefox when I reproduced the bug you describe here the inserter block list showed behind the modal overlay:

Screenshot 2019-11-04 at 15 01 10

Also I think it is kind of complicated to trigger this bug :) I had to click very carefully.

@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Nov 5, 2019

@draganescu Yep, I css hacked it in front in my screenshot for illustrative purposes.

… an issue where allowedBlocks for menu items are not registered until the menu item is clicked on
@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Nov 5, 2019

@noisysocks I've fixed the issue discussed above for the menu-item block by removing the conditional rendering of InnerBlocks and using CSS instead to hide it.

That's in bd8fb2e. Let me know if you're still happy!

@noisysocks

This comment has been minimized.

A cool trick I learned the other day is that you can use :not() for these kinds of situations.

&:not(.is-editing) .block-editor-inner-blocks {
	display: none;
}
Copy link
Member

noisysocks left a comment

I'm still happy!

@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Nov 6, 2019

Got that issue with Travis again where it shows as failed, when you click on it is says cancelled, then when you look at the actual tests they've all passed. Merging anyway.

@talldan talldan merged commit 73189e1 into master Nov 6, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Canceled
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Nov 6, 2019
@talldan talldan deleted the add/block-navigator-appender branch Nov 6, 2019
@talldan

This comment has been minimized.

Copy link
Contributor Author

talldan commented Nov 6, 2019

In terms of follow-ups, there's the issue where if the Navigator were enabled for another block, the inserter popover appears behind the modal. There's an existing issue covering this - #8468.

There's also the issue with conditional logic around InnerBlocks. I've created an issue for that - #18295.

@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
* Add appender to Block Navigator

* Use correct name for component

* Avoid selecting block on insert

* Add spoken message indicating block has been inserted

* Use CSS to hide inner blocks instead of conditional rendering. Solves an issue where allowedBlocks for menu items are not registered until the menu item is clicked on
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.