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 Navigation Links wrapper block #30430

Closed
wants to merge 2 commits into from
Closed

Conversation

tellthemachines
Copy link
Contributor

Description

Closes #24644.

Adds a new block that is a child of Navigation and parent of Navigation Links. It renders a <ul>, removing the need for the <ul> in the Navigation block.

When a Navigation block is created empty, Link List block is added automatically so users can start adding links immediately.

One thing that would benefit from design feedback is the appender situation, which can become confusing because either Links List or Navigation appender will show depending on which block is selected.

It would also be good to have an icon for this block; I used the Navigation Link one for now but it's not ideal.

This also doesn't solve the semantics issue with having Spacer blocks (<div>s) inside lists; it's probably best to fix that separately as this changeset is already quite big.

How has this been tested?

No automated tests yet; will update fixtures, deprecations etc. once this has been reviewed and we're all agreed on it.

Screenshots

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.
  • I've tested my changes with keyboard and screen readers.
  • 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 (please manually search all *.native.js files for terms that need renaming or removal).

@tellthemachines tellthemachines added [Block] Navigation Affects the Navigation Block Needs Design Feedback Needs general design feedback. labels Apr 1, 2021
@github-actions
Copy link

github-actions bot commented Apr 1, 2021

Size Change: +265 B (0%)

Total Size: 1.3 MB

Filename Size Change
build/block-library/index.js 147 kB +265 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.12 kB 0 B
build/annotations/index.js 2.93 kB 0 B
build/api-fetch/index.js 2.42 kB 0 B
build/autop/index.js 2.28 kB 0 B
build/blob/index.js 673 B 0 B
build/block-directory/index.js 6.61 kB 0 B
build/block-directory/style-rtl.css 993 B 0 B
build/block-directory/style.css 995 B 0 B
build/block-editor/index.js 118 kB 0 B
build/block-editor/style-rtl.css 13 kB 0 B
build/block-editor/style.css 13 kB 0 B
build/block-library/blocks/archives/editor-rtl.css 61 B 0 B
build/block-library/blocks/archives/editor.css 60 B 0 B
build/block-library/blocks/audio/editor-rtl.css 58 B 0 B
build/block-library/blocks/audio/editor.css 58 B 0 B
build/block-library/blocks/audio/style-rtl.css 112 B 0 B
build/block-library/blocks/audio/style.css 112 B 0 B
build/block-library/blocks/block/editor-rtl.css 161 B 0 B
build/block-library/blocks/block/editor.css 161 B 0 B
build/block-library/blocks/button/editor-rtl.css 475 B 0 B
build/block-library/blocks/button/editor.css 474 B 0 B
build/block-library/blocks/button/style-rtl.css 601 B 0 B
build/block-library/blocks/button/style.css 600 B 0 B
build/block-library/blocks/buttons/editor-rtl.css 315 B 0 B
build/block-library/blocks/buttons/editor.css 315 B 0 B
build/block-library/blocks/buttons/style-rtl.css 375 B 0 B
build/block-library/blocks/buttons/style.css 375 B 0 B
build/block-library/blocks/calendar/style-rtl.css 208 B 0 B
build/block-library/blocks/calendar/style.css 208 B 0 B
build/block-library/blocks/categories/editor-rtl.css 84 B 0 B
build/block-library/blocks/categories/editor.css 83 B 0 B
build/block-library/blocks/categories/style-rtl.css 79 B 0 B
build/block-library/blocks/categories/style.css 79 B 0 B
build/block-library/blocks/code/style-rtl.css 90 B 0 B
build/block-library/blocks/code/style.css 90 B 0 B
build/block-library/blocks/columns/editor-rtl.css 190 B 0 B
build/block-library/blocks/columns/editor.css 190 B 0 B
build/block-library/blocks/columns/style-rtl.css 422 B 0 B
build/block-library/blocks/columns/style.css 422 B 0 B
build/block-library/blocks/cover/editor-rtl.css 643 B 0 B
build/block-library/blocks/cover/editor.css 645 B 0 B
build/block-library/blocks/cover/style-rtl.css 1.22 kB 0 B
build/block-library/blocks/cover/style.css 1.22 kB 0 B
build/block-library/blocks/embed/editor-rtl.css 486 B 0 B
build/block-library/blocks/embed/editor.css 486 B 0 B
build/block-library/blocks/embed/style-rtl.css 401 B 0 B
build/block-library/blocks/embed/style.css 400 B 0 B
build/block-library/blocks/file/editor-rtl.css 301 B 0 B
build/block-library/blocks/file/editor.css 300 B 0 B
build/block-library/blocks/file/frontend.js 772 B 0 B
build/block-library/blocks/file/style-rtl.css 255 B 0 B
build/block-library/blocks/file/style.css 255 B 0 B
build/block-library/blocks/freeform/editor-rtl.css 2.45 kB 0 B
build/block-library/blocks/freeform/editor.css 2.45 kB 0 B
build/block-library/blocks/gallery/editor-rtl.css 704 B 0 B
build/block-library/blocks/gallery/editor.css 705 B 0 B
build/block-library/blocks/gallery/style-rtl.css 1.06 kB 0 B
build/block-library/blocks/gallery/style.css 1.05 kB 0 B
build/block-library/blocks/group/editor-rtl.css 160 B 0 B
build/block-library/blocks/group/editor.css 160 B 0 B
build/block-library/blocks/group/style-rtl.css 57 B 0 B
build/block-library/blocks/group/style.css 57 B 0 B
build/block-library/blocks/heading/editor-rtl.css 129 B 0 B
build/block-library/blocks/heading/editor.css 129 B 0 B
build/block-library/blocks/heading/style-rtl.css 76 B 0 B
build/block-library/blocks/heading/style.css 76 B 0 B
build/block-library/blocks/home-link/style-rtl.css 259 B 0 B
build/block-library/blocks/home-link/style.css 259 B 0 B
build/block-library/blocks/html/editor-rtl.css 281 B 0 B
build/block-library/blocks/html/editor.css 281 B 0 B
build/block-library/blocks/image/editor-rtl.css 717 B 0 B
build/block-library/blocks/image/editor.css 716 B 0 B
build/block-library/blocks/image/style-rtl.css 481 B 0 B
build/block-library/blocks/image/style.css 485 B 0 B
build/block-library/blocks/latest-comments/style-rtl.css 281 B 0 B
build/block-library/blocks/latest-comments/style.css 282 B 0 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B 0 B
build/block-library/blocks/latest-posts/editor.css 137 B 0 B
build/block-library/blocks/latest-posts/style-rtl.css 523 B 0 B
build/block-library/blocks/latest-posts/style.css 522 B 0 B
build/block-library/blocks/legacy-widget/editor-rtl.css 557 B 0 B
build/block-library/blocks/legacy-widget/editor.css 557 B 0 B
build/block-library/blocks/list/style-rtl.css 63 B 0 B
build/block-library/blocks/list/style.css 63 B 0 B
build/block-library/blocks/media-text/editor-rtl.css 176 B 0 B
build/block-library/blocks/media-text/editor.css 176 B 0 B
build/block-library/blocks/media-text/style-rtl.css 492 B 0 B
build/block-library/blocks/media-text/style.css 489 B 0 B
build/block-library/blocks/more/editor-rtl.css 434 B 0 B
build/block-library/blocks/more/editor.css 434 B 0 B
build/block-library/blocks/navigation-link/editor-rtl.css 617 B 0 B
build/block-library/blocks/navigation-link/editor.css 619 B 0 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B 0 B
build/block-library/blocks/navigation-link/style.css 94 B 0 B
build/block-library/blocks/navigation/editor-rtl.css 1.54 kB 0 B
build/block-library/blocks/navigation/editor.css 1.54 kB 0 B
build/block-library/blocks/navigation/style-rtl.css 1.78 kB 0 B
build/block-library/blocks/navigation/style.css 1.78 kB 0 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B 0 B
build/block-library/blocks/nextpage/editor.css 395 B 0 B
build/block-library/blocks/page-list/editor-rtl.css 310 B 0 B
build/block-library/blocks/page-list/editor.css 311 B 0 B
build/block-library/blocks/page-list/style-rtl.css 233 B 0 B
build/block-library/blocks/page-list/style.css 233 B 0 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B 0 B
build/block-library/blocks/paragraph/editor.css 157 B 0 B
build/block-library/blocks/paragraph/style-rtl.css 247 B 0 B
build/block-library/blocks/paragraph/style.css 248 B 0 B
build/block-library/blocks/post-author/editor-rtl.css 209 B 0 B
build/block-library/blocks/post-author/editor.css 209 B 0 B
build/block-library/blocks/post-author/style-rtl.css 183 B 0 B
build/block-library/blocks/post-author/style.css 184 B 0 B
build/block-library/blocks/post-comments-form/style-rtl.css 140 B 0 B
build/block-library/blocks/post-comments-form/style.css 140 B 0 B
build/block-library/blocks/post-comments/style-rtl.css 360 B 0 B
build/block-library/blocks/post-comments/style.css 359 B 0 B
build/block-library/blocks/post-content/editor-rtl.css 139 B 0 B
build/block-library/blocks/post-content/editor.css 139 B 0 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B 0 B
build/block-library/blocks/post-excerpt/editor.css 73 B 0 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B 0 B
build/block-library/blocks/post-excerpt/style.css 69 B 0 B
build/block-library/blocks/post-featured-image/editor-rtl.css 338 B 0 B
build/block-library/blocks/post-featured-image/editor.css 338 B 0 B
build/block-library/blocks/post-featured-image/style-rtl.css 119 B 0 B
build/block-library/blocks/post-featured-image/style.css 119 B 0 B
build/block-library/blocks/post-title/style-rtl.css 60 B 0 B
build/block-library/blocks/post-title/style.css 60 B 0 B
build/block-library/blocks/preformatted/style-rtl.css 103 B 0 B
build/block-library/blocks/preformatted/style.css 103 B 0 B
build/block-library/blocks/pullquote/editor-rtl.css 183 B 0 B
build/block-library/blocks/pullquote/editor.css 183 B 0 B
build/block-library/blocks/pullquote/style-rtl.css 318 B 0 B
build/block-library/blocks/pullquote/style.css 318 B 0 B
build/block-library/blocks/query-loop/editor-rtl.css 83 B 0 B
build/block-library/blocks/query-loop/editor.css 82 B 0 B
build/block-library/blocks/query-loop/style-rtl.css 315 B 0 B
build/block-library/blocks/query-loop/style.css 317 B 0 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B 0 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B 0 B
build/block-library/blocks/query-pagination/editor-rtl.css 270 B 0 B
build/block-library/blocks/query-pagination/editor.css 262 B 0 B
build/block-library/blocks/query-pagination/style-rtl.css 168 B 0 B
build/block-library/blocks/query-pagination/style.css 168 B 0 B
build/block-library/blocks/query-title/editor-rtl.css 86 B 0 B
build/block-library/blocks/query-title/editor.css 86 B 0 B
build/block-library/blocks/query/editor-rtl.css 131 B 0 B
build/block-library/blocks/query/editor.css 132 B 0 B
build/block-library/blocks/quote/style-rtl.css 169 B 0 B
build/block-library/blocks/quote/style.css 169 B 0 B
build/block-library/blocks/rss/editor-rtl.css 201 B 0 B
build/block-library/blocks/rss/editor.css 202 B 0 B
build/block-library/blocks/rss/style-rtl.css 290 B 0 B
build/block-library/blocks/rss/style.css 290 B 0 B
build/block-library/blocks/search/editor-rtl.css 189 B 0 B
build/block-library/blocks/search/editor.css 189 B 0 B
build/block-library/blocks/search/style-rtl.css 359 B 0 B
build/block-library/blocks/search/style.css 362 B 0 B
build/block-library/blocks/separator/editor-rtl.css 99 B 0 B
build/block-library/blocks/separator/editor.css 99 B 0 B
build/block-library/blocks/separator/style-rtl.css 251 B 0 B
build/block-library/blocks/separator/style.css 251 B 0 B
build/block-library/blocks/shortcode/editor-rtl.css 512 B 0 B
build/block-library/blocks/shortcode/editor.css 512 B 0 B
build/block-library/blocks/site-logo/editor-rtl.css 440 B 0 B
build/block-library/blocks/site-logo/editor.css 441 B 0 B
build/block-library/blocks/site-logo/style-rtl.css 154 B 0 B
build/block-library/blocks/site-logo/style.css 154 B 0 B
build/block-library/blocks/social-link/editor-rtl.css 164 B 0 B
build/block-library/blocks/social-link/editor.css 165 B 0 B
build/block-library/blocks/social-links/editor-rtl.css 796 B 0 B
build/block-library/blocks/social-links/editor.css 795 B 0 B
build/block-library/blocks/social-links/style-rtl.css 1.32 kB 0 B
build/block-library/blocks/social-links/style.css 1.33 kB 0 B
build/block-library/blocks/spacer/editor-rtl.css 308 B 0 B
build/block-library/blocks/spacer/editor.css 308 B 0 B
build/block-library/blocks/spacer/style-rtl.css 48 B 0 B
build/block-library/blocks/spacer/style.css 48 B 0 B
build/block-library/blocks/table/editor-rtl.css 478 B 0 B
build/block-library/blocks/table/editor.css 478 B 0 B
build/block-library/blocks/table/style-rtl.css 485 B 0 B
build/block-library/blocks/table/style.css 485 B 0 B
build/block-library/blocks/tag-cloud/editor-rtl.css 118 B 0 B
build/block-library/blocks/tag-cloud/editor.css 118 B 0 B
build/block-library/blocks/tag-cloud/style-rtl.css 94 B 0 B
build/block-library/blocks/tag-cloud/style.css 94 B 0 B
build/block-library/blocks/template-part/editor-rtl.css 551 B 0 B
build/block-library/blocks/template-part/editor.css 550 B 0 B
build/block-library/blocks/term-description/editor-rtl.css 90 B 0 B
build/block-library/blocks/term-description/editor.css 90 B 0 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B 0 B
build/block-library/blocks/text-columns/editor.css 95 B 0 B
build/block-library/blocks/text-columns/style-rtl.css 166 B 0 B
build/block-library/blocks/text-columns/style.css 166 B 0 B
build/block-library/blocks/verse/style-rtl.css 87 B 0 B
build/block-library/blocks/verse/style.css 87 B 0 B
build/block-library/blocks/video/editor-rtl.css 569 B 0 B
build/block-library/blocks/video/editor.css 570 B 0 B
build/block-library/blocks/video/style-rtl.css 169 B 0 B
build/block-library/blocks/video/style.css 169 B 0 B
build/block-library/common-rtl.css 1.26 kB 0 B
build/block-library/common.css 1.26 kB 0 B
build/block-library/editor-rtl.css 9.91 kB 0 B
build/block-library/editor.css 9.9 kB 0 B
build/block-library/reset-rtl.css 506 B 0 B
build/block-library/reset.css 507 B 0 B
build/block-library/style-rtl.css 10.2 kB 0 B
build/block-library/style.css 10.3 kB 0 B
build/block-library/theme-rtl.css 692 B 0 B
build/block-library/theme.css 693 B 0 B
build/block-serialization-default-parser/index.js 1.3 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 47.1 kB 0 B
build/components/index.js 188 kB 0 B
build/components/style-rtl.css 16.4 kB 0 B
build/components/style.css 16.3 kB 0 B
build/compose/index.js 9.92 kB 0 B
build/core-data/index.js 12.1 kB 0 B
build/customize-widgets/index.js 5.99 kB 0 B
build/customize-widgets/style-rtl.css 811 B 0 B
build/customize-widgets/style.css 813 B 0 B
build/data-controls/index.js 828 B 0 B
build/data/index.js 7.23 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 739 B 0 B
build/dom-ready/index.js 577 B 0 B
build/dom/index.js 4.62 kB 0 B
build/edit-navigation/index.js 13.6 kB 0 B
build/edit-navigation/style-rtl.css 2.83 kB 0 B
build/edit-navigation/style.css 2.83 kB 0 B
build/edit-post/classic-rtl.css 454 B 0 B
build/edit-post/classic.css 454 B 0 B
build/edit-post/index.js 334 kB 0 B
build/edit-post/style-rtl.css 6.84 kB 0 B
build/edit-post/style.css 6.83 kB 0 B
build/edit-site/index.js 26 kB 0 B
build/edit-site/style-rtl.css 4.79 kB 0 B
build/edit-site/style.css 4.78 kB 0 B
build/edit-widgets/index.js 12.5 kB 0 B
build/edit-widgets/style-rtl.css 3.02 kB 0 B
build/edit-widgets/style.css 3.03 kB 0 B
build/editor/index.js 38.4 kB 0 B
build/editor/style-rtl.css 3.95 kB 0 B
build/editor/style.css 3.95 kB 0 B
build/element/index.js 3.44 kB 0 B
build/escape-html/index.js 739 B 0 B
build/format-library/index.js 5.67 kB 0 B
build/format-library/style-rtl.css 637 B 0 B
build/format-library/style.css 639 B 0 B
build/hooks/index.js 1.76 kB 0 B
build/html-entities/index.js 628 B 0 B
build/i18n/index.js 3.73 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 1.65 kB 0 B
build/keycodes/index.js 1.43 kB 0 B
build/list-reusable-blocks/index.js 2.06 kB 0 B
build/list-reusable-blocks/style-rtl.css 629 B 0 B
build/list-reusable-blocks/style.css 628 B 0 B
build/media-utils/index.js 3.08 kB 0 B
build/navigation/index.js 2.85 kB 0 B
build/notices/index.js 1.07 kB 0 B
build/nux/index.js 2.31 kB 0 B
build/nux/style-rtl.css 718 B 0 B
build/nux/style.css 716 B 0 B
build/plugins/index.js 1.99 kB 0 B
build/primitives/index.js 1.03 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/react-i18n/index.js 923 B 0 B
build/redux-routine/index.js 2.82 kB 0 B
build/reusable-blocks/index.js 2.54 kB 0 B
build/reusable-blocks/style-rtl.css 225 B 0 B
build/reusable-blocks/style.css 225 B 0 B
build/rich-text/index.js 10.7 kB 0 B
build/server-side-render/index.js 1.64 kB 0 B
build/shortcode/index.js 1.68 kB 0 B
build/token-list/index.js 847 B 0 B
build/url/index.js 1.95 kB 0 B
build/viewport/index.js 1.28 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/widgets/index.js 1.66 kB 0 B
build/wordcount/index.js 1.24 kB 0 B

compressed-size-action

@talldan talldan added [Feature] Navigation Screen [Status] Blocked Used to indicate that a current effort isn't able to move forward labels Apr 1, 2021
@talldan
Copy link
Contributor

talldan commented Apr 1, 2021

Adding the 'blocked' status as currently this will break the Navigation editor.

We need a way to solve the problem outlined in #30006.

@jasmussen
Copy link
Contributor

jasmussen commented Apr 5, 2021

Thanks for working on this. This is what I see:

wrapper

I understand and support the goal of this PR, but the act of manually building a menu with the added wrapper is very poor
and un-intuitive when this is in place. While the nesting UI has come a long way, there are some fundamental user experience problems introduced here that we need a plan for, before we land it.

Going back to #24644, this seems to be the factor:

The accessibility advice is that only links should appear within a navigation's ul. Other blocks should render within the nav.

Right now the approach of a link wrapper entirely puts the burden on the user, having to learn why there needs to be an additional wrapper, and why manual page links work only inside that wrapper. In addition to this, there's an extra price to pay with the added nesting levels to fiddle with.

What alternatives to this approach exist? Can we add the ul automatically around adjacent menu items, only on the rendered frontend?

@tellthemachines
Copy link
Contributor Author

What alternatives to this approach exist? Can we add the ul automatically around adjacent menu items, only on the rendered frontend?

I'll look into it! Agree that having to navigate all these nesting levels is suboptimal 😬

@tellthemachines
Copy link
Contributor Author

Can we add the ul automatically around adjacent menu items, only on the rendered frontend?

I quickly put #30551 together to show what that would look like. It does go against the ideal of sharing markup between editor and front end, which feels a bit icky, and will require additional styling to make things look the same in both places.

Going back and re-reading all the history that got us to this point 😅 , I'm wondering why something like this hasn't been implemented (Cc. @ellatrix )

@talldan
Copy link
Contributor

talldan commented Apr 7, 2021

Going back and re-reading all the history that got us to this point 😅 , I'm wondering why something like this hasn't been implemented (Cc. @ellatrix )

I think that example was part of the inspiration for useInnerBlocksProps. This is from the example:

const { innerBlocks, innerBlocksWrapperProps } = useInnerBlocks( additionalProps );

The implementation now is a little different, but not massively so:

const { children, ...innerBlocksWrapperProps } = useInnerBlocksProps( additionalProps );

The difference (using a children prop) makes sense considering the props are nearly always spread onto an element.

@talldan
Copy link
Contributor

talldan commented Apr 7, 2021

Ah, just looking and I don't think it would work in the same way. It looks like the main difference is that children is a single InnerBlocks component, so can't be iterated.

@draganescu
Copy link
Contributor

In my mind the path forward here is to think of blocks that add themselves. When a user adds a link it will automatically add a links list block with a link in it. The links list block then only supports adding links as inner blocks.

Could we make a think like "required parent" and always wrap a block in the required parent?

Another question is if the menu link block should only exist in a links list block? In that case, even the pages block should probably use this links list block ... but that is another discussion.

I think adding a hack to fix broken editor html at render time, while looking good at UX level is really poor at implementation level. As we all agree that the main problem UX-wise with this block is the two step of adding a simple link, plus the termite swarm of appenders that one may end up with, this suggestion to have an auto-managed parent seems to fix this.

@gwwar
Copy link
Contributor

gwwar commented Apr 26, 2021

Overall, I'm pretty convinced that we do need some wrapper container here and one that users won't interact with. Eg they'd never be able to select the wrapper block, and we'd manage wrapper management (auto-add when needed/auto-remove when last child is deleted). To the user, the appender should be behave like we're adding direct children to the navigation block.

Not to blow up scope 😅 , but it feels like we'll probably need to end up taking on an attempt of #7694, with this block and maybe columns as first examples.

In terms of UX, we have a lot more options for selecting parents, that we'll need to 🤔 cases for:

List View Breadcrumb Persistent List View (Site Editor) Parent Block Selector
Screen Shot 2021-04-26 at 9 41 07 AM Screen Shot 2021-04-26 at 10 17 30 AM Screen Shot 2021-04-26 at 10 19 55 AM Screen Shot 2021-04-26 at 10 21 10 AM

In addition to:

  • Keyboard events
  • Focus/Touch events (make sure to target either the child content or the parent navigation block).
  • Testing edge cases in select/edit mode.

Do other folks agree on direction to help move this forward? I'm also happy to collaborate on this one @tellthemachines if that'd be helpful

@tellthemachines
Copy link
Contributor Author

Not to blow up scope 😅 , but it feels like we'll probably need to end up taking on an attempt of #7694, with this block and maybe columns as first examples.

#7694 does mention columns, but if we make column a passthrough block won't we lose the ability to set individual widths, as well as to rearrange columns by moving or dragging them?

I'm not sure how this would work for Navigation though, as the addition of a passthrough block would have to be conditional on the types of inner blocks in the nav, it would only be rendered around some of those inner blocks and we wouldn't be able to pre-wrap the block appender in a passthrough block as we don't know what type of block the user is going to try to append.

I might be missing something, but the problems we have here seem to be unique to Navigation, so the solution will likely have to be a unique one too.

@jasmussen
Copy link
Contributor

but if we make column a passthrough block won't we lose the ability to set individual widths, as well as to rearrange columns by moving or dragging them?

Not necessarily. The fact that you have to point and select individual columns in order to resize can be considered an aberration to similar interfaces, where I'd expect to see resize handles when the columns block itself is selected, and with input fields for precision or textual control surfaced in the inspector.

Now that the columns block supports showing a single column, to an extent at that point it's a group. While I don't think there are any plans for this outlined, I could imagine the transformation from a group to a columns block becoming so transparent as to be imperceptible to users. Specifically for the navigation block, such a group/columns interface could play a part of a future mega-menus interface. This is paused and an incomplete mockup, but it embraces a version of that idea.

To support Kerry's notion that this might need a holistic solution, I could see numerous blocks that have complicated markup that needs to be handled:

  • Table with nesting, where you should never have to select a thead, tbody, tfoot, or tr "wrapper" blocks. You should only ever have to select the table itself, and be able to set focus inside a table cell.
  • This may overlap with absorb toolbar iterations, but blocks like List and Quote when they receive nesting support, should ideally show both parent and child toolbars at the same time.
  • Form builder blocks, where items like fieldset or legend sections might be useful contextual wrappers but should mostly be handled by the block itself, rather than being user surfaced.

Ultimately it's about good block design, to surface only the controls and layers necessary to intuitively configure a block, and handling the markup for the user, rather than putting the onus on them. Whether that needs a passthrough block or not, I will defer to you, but the problem will almost certainly resurface outside of just the navigation block.

The table block is perhaps the ultimate expression of that, where the notoriously complex markup should be entirely handled by the block, and the user needing only to configure attributes on the table itself, and depending on contextual cell selection inside.

@gwwar
Copy link
Contributor

gwwar commented Apr 27, 2021

I might be missing something, but the problems we have here seem to be unique to Navigation, so the solution will likely have to be a unique one too.

I'm not tied to columns specifically, just 👀 other non-dynamic blocks that have similar issues. I'm also not sure if my suggestion will work.

I do think it's still worth investigating a good timeboxed exploration (maybe a day or two) to sketch out if this is possible, or if we quickly run into some major roadblocks. If we see the latter, we can document those and then I think more folks would be willing to continue with a more specific solution for the navigation block.

I'm definitely open to other ideas too if something comes to mind. I think the two two main things to get right are simple UX interactions for folks and more correct markup. Together the requirements makes it really tricky, but there might be a lot of ways to implement that.

@ellatrix
Copy link
Member

Can we add the ul automatically around adjacent menu items, only on the rendered frontend?

I quickly put #30551 together to show what that would look like. It does go against the ideal of sharing markup between editor and front end, which feels a bit icky, and will require additional styling to make things look the same in both places.

Going back and re-reading all the history that got us to this point 😅 , I'm wondering why something like this hasn't been implemented (Cc. @ellatrix )

It's been tried by @ZebulanStanphill, but it needs changes to the parser too to be able to do it.

@tellthemachines
Copy link
Contributor Author

The fact that you have to point and select individual columns in order to resize can be considered an aberration to similar interfaces, where I'd expect to see resize handles when the columns block itself is selected, and with input fields for precision or textual control surfaced in the inspector.

This is a great point, and would make for a much nicer interface.

Thinking out loud a bit here:

I'm not sure what the scenario is with List and Quote blocks, or with Table block for that matter - should Tables ever be able to accept nested blocks? That would get us dangerously close to enabling table-based layouts 😬

But the common point I'm seeing between Columns, Navigation and a possible Form block are that, in all of them, we want to be able to group their inner blocks in certain ways. For columns, we want inner blocks in Col A to be separate from those in Col B, and having those intermediate Column blocks gives us a way to do that. It's better for all the intermediate block logic to be invisible in the UI, but users still get to pick which column to nest a block in.

For a Form block, we could provide the ability to create semantic groupings of elements, which in the markup would be fieldsets with legends, but users would still get to pick which grouping to drop a particular block in, and name that grouping.

For Navigation though, we want particular types of inner blocks - Links and Home block - to be grouped in a specific set, and other types - Search, Social - to be grouped in a different set, but we don't want users to pick which set their block goes in. We want them to be able to add their blocks in whatever order, and then we make sense of which set to put each block in behind the scenes.

So these problems are really opposites: for one, we allow users to pick the bucket they want to put whatever block in. For the other, we only give users one bucket, and then we go through and sort the blocks they put in according to type.

(With the mega menu we'd potentially have a multi-bucket interface, but we'd still need to sort through each bucket to add the semantics afterwards)

Which is why I'm inlined to a different solution for the Navigation (and potentially for any other block that might have a similar single-bucket logic, though I can't think of any offhand) than for Columns/Form/etc. and defining the markup around the blocks after they're added seems simpler than wrapping them in intermediate blocks.

Thoughts?

@talldan
Copy link
Contributor

talldan commented Apr 29, 2021

should Tables ever be able to accept nested blocks? That would get us dangerously close to enabling table-based layouts 😬

It wouldn't have to accept every kind of block. It's more about having a cleaner separation of concerns between sections/rows/cells.

An example of why it'd be useful is that at the moment a cell can't take advantage of any of the code reuse between blocks (e.g. block supports settings), a feature has to be added in a custom way to the table block and apply to all cells.

@talldan
Copy link
Contributor

talldan commented Apr 29, 2021

For Navigation though, we want particular types of inner blocks - Links and Home block - to be grouped in a specific set, and other types - Search, Social - to be grouped in a different set, but we don't want users to pick which set their block goes in. We want them to be able to add their blocks in whatever order, and then we make sense of which set to put each block in behind the scenes.

My understanding is that Search/Social shouldn't be in a set, they're just children of the nav block.

So these problems are really opposites: for one, we allow users to pick the bucket they want to put whatever block in. For the other, we only give users one bucket, and then we go through and sort the blocks they put in according to type.

I think there's still commonalities between column and links. Quoting from #7694, the passthrough idea is that it 'omits most of the interaction points of a block if it is not intended to be mutated directly, instead serving only as an intermediary for nested block wrapping'. So the block can't be interacted with in a lot of the usual ways. I think it's worth keeping that idea pure and distinct from some of the other ideas for the links block. E.g. that its inner block types are automatically surfaced to the parent navigation block, and are grouped automatically when those inner block types are added. Those things can be distinct from the passthrough idea.

Also, just want to mention that I'm not championing this idea, I can see there are potential problems and inconsistencies.

Which is why I'm inlined to a different solution for the Navigation (and potentially for any other block that might have a similar single-bucket logic, though I can't think of any offhand) than for Columns/Form/etc. and defining the markup around the blocks after they're added seems simpler than wrapping them in intermediate blocks.

I'm not sure what you mean by 'single-bucket logic'. I think you could still have two or more separate lists of links in a navigation block, potentially separated by a header.

I think it's more that consecutive blocks of a certain type get automatically wrapped with HTML list markup.

I feel like that needs a technical prototype more than anything to determine how feasible it is.

and potentially for any other block that might have a similar single-bucket logic, though I can't think of any offhand

I remember the gallery had/has the same problem for its switch to inner blocks, not sure whether it was solved there?

@tellthemachines
Copy link
Contributor Author

My understanding is that Search/Social shouldn't be in a set, they're just children of the nav block.

By "set" I mean they are in the group of blocks that are treated as direct children of Nav, as opposed to the group that needs to be wrapped in a list.

I'm not sure what you mean by 'single-bucket logic'. I think you could still have two or more separate lists of links in a navigation block, potentially separated by a header.

By "single-bucket logic" I mean a block that only has one place where users can add blocks, as opposed to e.g. Columns where they can add blocks in each column. The point is we sort through the content of that bucket afterwards, and separate it out into semantic groupings. (That doesn't exclude that we might have more than one list of links, but we have to handle that logic, not the user.)

@jasmussen
Copy link
Contributor

Wherever we land, I would expect this user experience to remain intact:

nav mover

@tellthemachines
Copy link
Contributor Author

Wherever we land, I would expect this user experience to remain intact:

Hmm, if we use passthrough blocks this will require seamlessly moving items between blocks.

@ellatrix
Copy link
Member

Allow my to attempt a PR :)

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Before we continue down this path, it would be good to have a discussion on lists in the nav block in general (#31827).

@getdave
Copy link
Contributor

getdave commented May 17, 2021

Let me first say I'm not at all against this block. I recognise why it might be necessary and it's great to see a PoC 👍

However, reading around the problem space I think we should also consider how adding this block will effect our ability to serialize to the existing Menus.

#30006

For example, if we use a Link wrapper block then how will this serialize to nav_menu_items? More importantly, how could we restore the wrapper block when we parse from nav_menu_items? I can think of some ways but they become pretty involved rather quickly.

@getdave
Copy link
Contributor

getdave commented May 17, 2021

Allow my to attempt a PR :)

@ellatrix Do you have a PR to allow items to be moved seamlessly across blocks of matching types? That sounds interesting!

@draganescu draganescu mentioned this pull request May 17, 2021
7 tasks
@getdave getdave force-pushed the try/nav-links-block branch from 2e17725 to bec2a78 Compare May 18, 2021 14:42
@getdave
Copy link
Contributor

getdave commented May 18, 2021

@tellthemachines I've rebased the branch as it had accumulated a fair few conflicts. Hopefully that helps a bit?

@tellthemachines
Copy link
Contributor Author

As per discussion on #31951, I'm closing this in favour of #30551.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block Needs Design Feedback Needs general design feedback. [Status] Blocked Used to indicate that a current effort isn't able to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation: Try adding Links block
7 participants