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 navigator for nav block #17265

Merged
merged 5 commits into from Sep 5, 2019

Conversation

@talldan
Copy link
Contributor

commented Aug 30, 2019

Description

Fixes #16812, adds Block Navigation to the Navigation Block.

I've called it 'Block Navigator', but the name clash seems like a little issue that might need to be resolved.

Changes:

  • Extract and export BlockNavigationList as a component from @wordpress/block-editor
  • Add new BlockNavigatorModal component in the navigation block's folder and integrate.

How has this been tested?

Manual testing

Screenshots

Screen Shot 2019-08-30 at 1 51 42 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.
<BlockNavigationList
blocks={ [ block ] }
selectedBlockClientId={ selectedBlockClientId }
selectBlock={ selectBlock }

This comment has been minimized.

Copy link
@talldan

talldan Aug 30, 2019

Author Contributor

Selecting the block works well in the current Block Navigation Menu accessible from the top toolbar. The PopOver closes and the correct block becomes selected.

In a modal this makes less sense. Currently it makes the modal close (though not sure why). I think this is the biggest issue with the experience as it stands.

This comment has been minimized.

Copy link
@talldan

talldan Aug 30, 2019

Author Contributor

Oh, I suppose it makes sense that selectBlock is causing the focus to shift and that's closing the modal.

This comment has been minimized.

Copy link
@draganescu

draganescu Aug 30, 2019

Contributor

The navigator view is not meant for navigation only, it is, or it should be a way to build and edit the menu. We could make a double click action or some other way to select a deeply nested item and return to the navigation block edit view, for example a la dev tools "inspect" :)

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Sep 2, 2019

Contributor

I'm not sold on using a modal for this functionality as it stands. What @draganescu is describing - that it should be a way to build and edit the menu - would make more sense as a modal, but on the other hand shouldn't the block itself provide that drag/drop/edit capability already?
If this navigator is replicating the top one, it would be better to have it as a dropdown, exactly like the top one.

Another thing I notice with the modal is when closing it with esc focus goes to the block wrapper (not sure if that's what it's called - I mean the element that gets focus when navigating blocks with the arrow keys) whereas I would expect it to go back to the initial button, or at least to the next focusable element.

This comment has been minimized.

Copy link
@talldan

talldan Sep 2, 2019

Author Contributor

The idea is definitely that this is a data view, and will be used for re-arranging things, potentially adding new blocks. I plan to do this incrementally though and not on the same PR. It's a good question about where block selection fits into this modal.

Another thing I notice with the modal is when closing it with esc focus goes to the block wrapper (not sure if that's what it's called - I mean the element that gets focus when navigating blocks with the arrow keys) whereas I would expect it to go back to the initial button, or at least to the next focusable element.

Good catch, will see if I can figure out what's going on.

This comment has been minimized.

Copy link
@talldan

talldan Sep 2, 2019

Author Contributor

esc was indeed triggering navigation mode, and with the way the escape key event was working for modals (as a global event binding) there was no way of stopping propagation.

I've re-arranged things in fc2d299, and it's an improvement, but it still seems like the wrong icon is focused when pressing escape.

I'll have to bring those changes to the modal to a separate PR and will continue looking into the issue.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Sep 3, 2019

Contributor

Hmm yeah, seems like focus is always returned to the previous focusable element rather than the next one. Agree it's a lot better than it was 😄
Apart from this modal the same problem was also affecting the one that pops up for "Resolve" on an invalid block. That one is also now returning focus to the previous focusable element.

This comment has been minimized.

Copy link
@talldan

talldan Sep 3, 2019

Author Contributor

I have a fix locally and some general modal fixes on #17297. Will bring them all together once that's merged.

Had a look at the that invalid block modal, and it looks like when the modal is open, the button that opens it is no longer rendered, so I think that causes the focus return not to work.

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

Also noted that when you use the Top Toolbar option, the doubling up of the icon might be a problem:
Screen Shot 2019-08-30 at 2 04 42 pm

@talldan talldan changed the title Add/navigator for nav block Add navigator for nav block Aug 30, 2019

return (
<Fragment>
{ isNavigationListOpen && (
<Modal

This comment has been minimized.

Copy link
@draganescu

draganescu Aug 30, 2019

Contributor

I would have expected that the Navigation block would use both modal and the Block navigator to make up the UX in such way. Is this better to have the block navigator be a modal?

This comment has been minimized.

Copy link
@talldan

talldan Sep 2, 2019

Author Contributor

Yeah, it might be good to not have the modal render as a child of the block toolbar, considering how the toolbar likes to unmount quite a lot.

Also keen to not add too much code to the main Navigation Menu edit component though or make this any more complicated than it needs to be.

This comment has been minimized.

Copy link
@talldan

talldan Sep 4, 2019

Author Contributor

I've switched to using a hook for the block navigator and I'm now rendering the modal outside of the toolbar. It probably won't stay this way. I could imagine that if the navigator is something we'd consider using for other blocks, then the modal would be moved to the editor itself and be opened using an action.

This is a pragmatic solution for now.

<BlockNavigationList
blocks={ [ block ] }
selectedBlockClientId={ selectedBlockClientId }
selectBlock={ selectBlock }

This comment has been minimized.

Copy link
@draganescu

draganescu Aug 30, 2019

Contributor

The navigator view is not meant for navigation only, it is, or it should be a way to build and edit the menu. We could make a double click action or some other way to select a deeply nested item and return to the navigation block edit view, for example a la dev tools "inspect" :)

@mtias

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

This is a great start, thanks!

@karmatosed

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

This is really great, from a design review perspective it work as expected so the feedback is 👍. I'll give a review to tick that box, but it does make me think iterating this interface will help but that's separate from this issue, it's in which is great! Thanks for all the awesome work here @talldan.

@karmatosed karmatosed self-requested a review Aug 31, 2019

@karmatosed
Copy link
Member

left a comment

Based on using an existing design, approving the design and removing design feedback label.

@tellthemachines
Copy link
Contributor

left a comment

Not sure how useful this view is for the nav, given that all the items have the same name. It'll be quite hard to make sense of anything but a very simple structure with very few elements.
Screen Shot 2019-09-02 at 1 18 56 pm

Also, had a look at this in IE 😅

Screen Shot 2019-09-02 at 11 01 09 am

This seems to be caused by block-editor-block-toolbar__slot being set to display: inline-block, and I think that is because having position: absolute set on its parent breaks the layout even more if the slot is set to flex. I wonder if we really need the toolbar to be absolutely positioned though?

@@ -122,6 +122,10 @@ Undocumented declaration.

Undocumented declaration.

<a name="BlockNavigationList" href="#BlockNavigationList">#</a> **BlockNavigationList**

Undocumented declaration.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Sep 2, 2019

Contributor

I notice there's a bunch of Undocumented declarations in this file, shouldn't they be documented?

This comment has been minimized.

Copy link
@talldan

talldan Sep 4, 2019

Author Contributor

I don't know to be honest. I think originally undocumented components didn't show up in the docs.

I don't particularly want to encourage outside use of this component while it's newly extracted and only being used for an experimental block. Perhaps I should make the component itself experimental. 🤔

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Sep 5, 2019

Contributor

Maybe something changed with how the docs are generated. Probably not an issue for this PR!

<BlockNavigationList
blocks={ [ block ] }
selectedBlockClientId={ selectedBlockClientId }
selectBlock={ selectBlock }

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Sep 2, 2019

Contributor

I'm not sold on using a modal for this functionality as it stands. What @draganescu is describing - that it should be a way to build and edit the menu - would make more sense as a modal, but on the other hand shouldn't the block itself provide that drag/drop/edit capability already?
If this navigator is replicating the top one, it would be better to have it as a dropdown, exactly like the top one.

Another thing I notice with the modal is when closing it with esc focus goes to the block wrapper (not sure if that's what it's called - I mean the element that gets focus when navigating blocks with the arrow keys) whereas I would expect it to go back to the initial button, or at least to the next focusable element.

@talldan talldan requested review from chrisvanpatten and gziolo as code owners Sep 2, 2019

@talldan talldan referenced this pull request Sep 2, 2019
5 of 5 tasks complete
@mtias

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

@tellthemachines that was discussed in slack (#design) last week. The plan is to come up with some sort of concept where a block can specify which of its attributes could be used as "display name". Menu Item could say label is its display name, while a Heading could say its content can be a display name. That way the navigator would become a lot more useful not just for the menu item but for may other blocks.

@talldan talldan force-pushed the add/navigator-for-nav-block branch from fc2d299 to 6fe0504 Sep 4, 2019

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Now that #17297 has been merged, the issue with navigator mode being triggered should be resolved and focus should be returned to the correct place after closing the modal.

I think it's good for a re-review.

@tellthemachines
Copy link
Contributor

left a comment

Code looks good! Left a couple of tiny questions, more out of curiosity than anything else.

@@ -122,6 +122,10 @@ Undocumented declaration.

Undocumented declaration.

<a name="BlockNavigationList" href="#BlockNavigationList">#</a> **BlockNavigationList**

Undocumented declaration.

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Sep 5, 2019

Contributor

Maybe something changed with how the docs are generated. Probably not an issue for this PR!

* Safari+VoiceOver won't announce the list otherwise.
*/
/* eslint-disable jsx-a11y/no-redundant-roles */
<ul className="editor-block-navigation__list block-editor-block-navigation__list" role="list">

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Sep 5, 2019

Contributor

Do we need the two almost identical classnames? Only block-editor-block-navigation__list has CSS attached.

<li key={ block.clientId }>
<div className="editor-block-navigation__item block-editor-block-navigation__item">
<Button
className={ classnames( 'editor-block-navigation__item-button block-editor-block-navigation__item-button', {

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Sep 5, 2019

Contributor

Hmm I see there's a pattern. Are some of these classes there for backward compatibility reasons or something?

This comment has been minimized.

Copy link
@talldan

talldan Sep 5, 2019

Author Contributor

@tellthemachines Yep, it's for backwards compatibility. Previously all these components were in the editor package, but were moved to the block-editor package.

@talldan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Thanks for the code reviews. 🎉

@talldan talldan merged commit 8dc1de9 into master Sep 5, 2019

2 checks passed

pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details

@talldan talldan deleted the add/navigator-for-nav-block branch Sep 5, 2019

@talldan talldan added this to the Gutenberg 6.5 milestone Sep 5, 2019

@noisysocks

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Nice work on this! 😍

@@ -12,6 +12,7 @@ export { default as BlockEdit } from './block-edit';
export { default as BlockFormatControls } from './block-format-controls';
export { default as BlockIcon } from './block-icon';
export { default as BlockNavigationDropdown } from './block-navigation/dropdown';
export { default as BlockNavigationList } from './block-navigation/list';

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 14, 2019

Contributor

How confident are we to expose this as a public API?

This comment has been minimized.

Copy link
@talldan

talldan Sep 14, 2019

Author Contributor

@youknowriad It is difficult to see how this block navigator will evolve at the moment. While the api for this component is pretty clean, I could imagine it not being needed to be exposed in the future as a component/api more tailored for individual blocks is introduced.

I suppose a strategy could be to make a duplicate component in the block library for the navigation block, develop that, and then once it's a bit clearer look at moving those improvements back to the block-editor package.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 15, 2019

Contributor

Maybe it's better to mark it "experimental" for the moment right?

This comment has been minimized.

Copy link
@talldan

talldan Sep 15, 2019

Author Contributor

@youknowriad here's a pr for that #17446

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 15, 2019

Contributor

Awesome thank you.

@phpbits phpbits referenced this pull request Sep 18, 2019
0 of 2 tasks complete
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.