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

Experiment: dedicated menu and menu item blocks for navigation editor #34496

Closed
wants to merge 10 commits into from

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Sep 2, 2021

Description

Something I've wanted to try for a while, is removing the dependency on the navigation block in the navigation editor.

Instead, this introduces a menu block and a menu item block, which is intended to correlate with the same functionality provided by the current WordPress menu editor.

This has been a bit of an exercise to see what gains there are from such an approach.

I've found it removes a lot of complexity.

The downside is quite a bit of code duplication, particularly for menu items / navigation links:

  • Setting up variations.
  • Converting pages/menu items to blocks in the placeholder.

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.
  • 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).

@talldan
Copy link
Contributor Author

talldan commented Sep 2, 2021

Thinking about this some more, I don't think a 'Menu' block is actually needed, the editor can just use menu items directly. I might consider removing 'Menu'.

@github-actions
Copy link

github-actions bot commented Sep 3, 2021

Size Change: +5.13 kB (0%)

Total Size: 1.05 MB

Filename Size Change
build/edit-navigation/index.min.js 18.9 kB +5.26 kB (+39%) 🚨
build/edit-navigation/style-rtl.css 3.08 kB -68 B (-2%)
build/edit-navigation/style.css 3.07 kB -67 B (-2%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 931 B
build/admin-manifest/index.min.js 1.09 kB
build/annotations/index.min.js 2.7 kB
build/api-fetch/index.min.js 2.19 kB
build/autop/index.min.js 2.08 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.2 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/index.min.js 119 kB
build/block-editor/style-rtl.css 13.8 kB
build/block-editor/style.css 13.8 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 474 B
build/block-library/blocks/button/editor.css 474 B
build/block-library/blocks/button/style-rtl.css 600 B
build/block-library/blocks/button/style.css 600 B
build/block-library/blocks/buttons/editor-rtl.css 315 B
build/block-library/blocks/buttons/editor.css 315 B
build/block-library/blocks/buttons/style-rtl.css 370 B
build/block-library/blocks/buttons/style.css 370 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 131 B
build/block-library/blocks/code/theme.css 131 B
build/block-library/blocks/columns/editor-rtl.css 194 B
build/block-library/blocks/columns/editor.css 193 B
build/block-library/blocks/columns/style-rtl.css 474 B
build/block-library/blocks/columns/style.css 475 B
build/block-library/blocks/cover/editor-rtl.css 666 B
build/block-library/blocks/cover/editor.css 670 B
build/block-library/blocks/cover/style-rtl.css 1.23 kB
build/block-library/blocks/cover/style.css 1.23 kB
build/block-library/blocks/embed/editor-rtl.css 488 B
build/block-library/blocks/embed/editor.css 488 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 925 B
build/block-library/blocks/gallery/editor.css 929 B
build/block-library/blocks/gallery/style-rtl.css 1.61 kB
build/block-library/blocks/gallery/style.css 1.61 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 70 B
build/block-library/blocks/group/theme.css 70 B
build/block-library/blocks/heading/style-rtl.css 114 B
build/block-library/blocks/heading/style.css 114 B
build/block-library/blocks/home-link/style-rtl.css 247 B
build/block-library/blocks/home-link/style.css 247 B
build/block-library/blocks/html/editor-rtl.css 283 B
build/block-library/blocks/html/editor.css 284 B
build/block-library/blocks/image/editor-rtl.css 728 B
build/block-library/blocks/image/editor.css 728 B
build/block-library/blocks/image/style-rtl.css 482 B
build/block-library/blocks/image/style.css 487 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 488 B
build/block-library/blocks/media-text/style.css 485 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 489 B
build/block-library/blocks/navigation-link/editor.css 488 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation/editor-rtl.css 1.72 kB
build/block-library/blocks/navigation/editor.css 1.72 kB
build/block-library/blocks/navigation/style-rtl.css 1.42 kB
build/block-library/blocks/navigation/style.css 1.41 kB
build/block-library/blocks/navigation/view.min.js 2.52 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 310 B
build/block-library/blocks/page-list/editor.css 310 B
build/block-library/blocks/page-list/style-rtl.css 241 B
build/block-library/blocks/page-list/style.css 241 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 261 B
build/block-library/blocks/paragraph/style.css 261 B
build/block-library/blocks/post-author/editor-rtl.css 210 B
build/block-library/blocks/post-author/editor.css 210 B
build/block-library/blocks/post-author/style-rtl.css 182 B
build/block-library/blocks/post-author/style.css 181 B
build/block-library/blocks/post-comments-form/style-rtl.css 140 B
build/block-library/blocks/post-comments-form/style.css 140 B
build/block-library/blocks/post-comments/style-rtl.css 360 B
build/block-library/blocks/post-comments/style.css 359 B
build/block-library/blocks/post-content/editor-rtl.css 138 B
build/block-library/blocks/post-content/editor.css 138 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 398 B
build/block-library/blocks/post-featured-image/editor.css 398 B
build/block-library/blocks/post-featured-image/style-rtl.css 143 B
build/block-library/blocks/post-featured-image/style.css 143 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 378 B
build/block-library/blocks/post-template/style.css 379 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 60 B
build/block-library/blocks/post-title/style.css 60 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 378 B
build/block-library/blocks/pullquote/style.css 378 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 270 B
build/block-library/blocks/query-pagination/editor.css 262 B
build/block-library/blocks/query-pagination/style-rtl.css 168 B
build/block-library/blocks/query-pagination/style.css 168 B
build/block-library/blocks/query-title/editor-rtl.css 85 B
build/block-library/blocks/query-title/editor.css 85 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 187 B
build/block-library/blocks/quote/style.css 187 B
build/block-library/blocks/quote/theme-rtl.css 220 B
build/block-library/blocks/quote/theme.css 222 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 374 B
build/block-library/blocks/search/style.css 375 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 250 B
build/block-library/blocks/separator/style.css 250 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 462 B
build/block-library/blocks/site-logo/editor.css 464 B
build/block-library/blocks/site-logo/style-rtl.css 153 B
build/block-library/blocks/site-logo/style.css 153 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 165 B
build/block-library/blocks/social-link/editor.css 165 B
build/block-library/blocks/social-links/editor-rtl.css 812 B
build/block-library/blocks/social-links/editor.css 811 B
build/block-library/blocks/social-links/style-rtl.css 1.33 kB
build/block-library/blocks/social-links/style.css 1.33 kB
build/block-library/blocks/spacer/editor-rtl.css 307 B
build/block-library/blocks/spacer/editor.css 307 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 146 B
build/block-library/blocks/tag-cloud/style.css 146 B
build/block-library/blocks/template-part/editor-rtl.css 636 B
build/block-library/blocks/template-part/editor.css 635 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/term-description/editor-rtl.css 90 B
build/block-library/blocks/term-description/editor.css 90 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 571 B
build/block-library/blocks/video/editor.css 572 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 1.29 kB
build/block-library/common.css 1.29 kB
build/block-library/editor-rtl.css 9.98 kB
build/block-library/editor.css 9.96 kB
build/block-library/index.min.js 150 kB
build/block-library/reset-rtl.css 527 B
build/block-library/reset.css 527 B
build/block-library/style-rtl.css 10.6 kB
build/block-library/style.css 10.6 kB
build/block-library/theme-rtl.css 658 B
build/block-library/theme.css 663 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/blocks/index.min.js 46.9 kB
build/components/index.min.js 209 kB
build/components/style-rtl.css 15.8 kB
build/components/style.css 15.8 kB
build/compose/index.min.js 10.2 kB
build/core-data/index.min.js 12.4 kB
build/customize-widgets/index.min.js 11.1 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 614 B
build/data/index.min.js 7.1 kB
build/date/index.min.js 31.5 kB
build/deprecated/index.min.js 428 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.53 kB
build/edit-post/classic-rtl.css 492 B
build/edit-post/classic.css 494 B
build/edit-post/index.min.js 28.9 kB
build/edit-post/style-rtl.css 7.2 kB
build/edit-post/style.css 7.19 kB
build/edit-site/index.min.js 26.3 kB
build/edit-site/style-rtl.css 5.07 kB
build/edit-site/style.css 5.07 kB
build/edit-widgets/index.min.js 16 kB
build/edit-widgets/style-rtl.css 4.06 kB
build/edit-widgets/style.css 4.06 kB
build/editor/index.min.js 37.6 kB
build/editor/style-rtl.css 3.74 kB
build/editor/style.css 3.73 kB
build/element/index.min.js 3.17 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 5.36 kB
build/format-library/style-rtl.css 668 B
build/format-library/style.css 669 B
build/hooks/index.min.js 1.55 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.6 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.49 kB
build/keycodes/index.min.js 1.25 kB
build/list-reusable-blocks/index.min.js 1.85 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.88 kB
build/notices/index.min.js 845 B
build/nux/index.min.js 2.03 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.83 kB
build/primitives/index.min.js 921 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 B
build/redux-routine/index.min.js 2.63 kB
build/reusable-blocks/index.min.js 2.28 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.6 kB
build/server-side-render/index.min.js 1.32 kB
build/shortcode/index.min.js 1.48 kB
build/token-list/index.min.js 562 B
build/url/index.min.js 1.74 kB
build/viewport/index.min.js 1.02 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 6.37 kB
build/widgets/style-rtl.css 1.05 kB
build/widgets/style.css 1.05 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

@talldan
Copy link
Contributor Author

talldan commented Sep 3, 2021

I've implemented the placeholder from #23953 in this PR as it was very easy to do without the technical blockers of having to extend the nav block.

@talldan talldan force-pushed the try/dedicated-menu-block-for-navigation-editor branch from 9c5bff4 to 9c6f594 Compare September 3, 2021 06:18
@talldan
Copy link
Contributor Author

talldan commented Sep 3, 2021

I think this offers a lot of opportunities:

  • Exploring using useEntityProp for editing menu item properties (similar to how a block like PostTitle works), which wouldn't be possible in the navigation link block. This architecture would make it easy to save only edited menu items.
  • There has been significant feedback about the the lack of UI clarity for nested menus. This can be iterated more quickly, without having to work around how the nav block works.
  • It'll be easier to show validation issues for menu items, including if there's an error from the REST API.
  • The dedicated blocks have more scope for supporting backwards compatibility (especially around extensibility).
  • The placeholder and block styles are already significantly improved in this PR, and it didn't take much effort.
  • This PR already removes a lot of the more fragile code in the navigation editor, and there are more opportunities
  • It'd be interesting to look into completely removing the 'Menu' block, thinking about it the block might be unnecessary, a user only edits menu items.
  • The navigation block itself can be iterated on without concern for navigation editor support.

I think there could be some questions:

  • Are these dedicated block less portable? I don't think so, as copy/paste between editors could still be handled using something like a raw transform. Or the transform system could be extended to support converting from an unsupported block to a supported one.
  • How would the theme opt-in feature work? I think if the user has an existing 'Menu' block and the theme opt-in is detected, the editor could provide an option to convert/upgrade to a navigation block. The menu block also gives the user the option to stick with their classic menu if that's not what they desire.

@getdave
Copy link
Contributor

getdave commented Sep 3, 2021

Thanks for exploring this 🏅. It works well at first glance.

Here are some responses. I feel like we'll need to consider this very carefully...

  • Exploring using useEntityProp for editing menu item properties (similar to how a block like PostTitle works), which wouldn't be possible in the navigation link block. This architecture would make it easy to save only edited menu items.

I'm not clear on how/why it's not possible? What benefit does it bring saving only edited items? Would you say this is a major win or a nice side effect?

  • There has been significant feedback about the the lack of UI clarity for nested menus. This can be iterated more quickly, without having to work around how the nav block works.

I see now that this PR drops the Nav Block entirely within the Nav Editor. Is this effectively a "Classic" Navigation block?

  • It'll be easier to show validation issues for menu items, including if there's an error from the REST API.

How is it easier? I'm genuinely curious.

  • The dedicated blocks have more scope for supporting backwards compatibility (especially around extensibility).

How do they have better scope for support this? What specifically would be easier?

  • The placeholder and block styles are already significantly improved in this PR, and it didn't take much effort.

I agree the placeholder was much nicer 👍

  • This PR already removes a lot of the more fragile code in the navigation editor, and there are more opportunities

I'm guessing this is mostly not having to disable all the "unwanted" Nav block features right?

  • It'd be interesting to look into completely removing the 'Menu' block, thinking about it the block might be unnecessary, a user only edits menu items.

I'm confused. Are you suggesting a Menu Item blocks as children of the Navigation block?

  • The navigation block itself can be iterated on without concern for navigation editor support.

Only if we don't use the Navigation block and utilise this new Menu block instead. Otherwise we'll have to make sure Nav block changes work for the Menu block.

  • How would the theme opt-in feature work? I think if the user has an existing 'Menu' block and the theme opt-in is detected, the editor could provide an option to convert/upgrade to a navigation block. The menu block also gives the user the option to stick with their classic menu if that's not what they desire.

What happens if we decide to allow for block based themes to opt into block based rendering of menus. Wouldn't Themers now need to write styles for the Menu block as well as the Navigation block? It's another area of inconsistency and complexity to be considered.

@talldan
Copy link
Contributor Author

talldan commented Sep 4, 2021

I'm not clear on how/why it's not possible? What benefit does it bring saving only edited items? Would you say this is a major win or a nice side effect?

This is something that will be necessary before shipping to core, and this way will be a much cleaner architecture, the best option we have now is to diff each menu item for changes.

Is this effectively a "Classic" Navigation block?

Yes, but I think we're at the point where the navigation editor changes the navigation block so much that it's unrecognisable from what you see in other editors, I don't think users will perceive much of a difference in how the editor works.

How is it easier? I'm genuinely curious.

At the moment there are two types of validation that would ideally be implemented, client side validation (catching issue before sending menu items to the REST API) and the ability to show validation errors from the REST API. The navigation block could have its own client side validation (#31716), but there's no reason for this to have the same specification as what's required in the navigation editor because the block doesn't use the REST API by default. Showing errors from the server is even more difficult, the editor needs to hook into the link block which has limitations on what can be achieved.

How do they have better scope for support this? What specifically would be easier?

It's just a matter of having full control over the block. Whereas we want to avoid adding too many backwards compatible features to the navigation and link blocks, because its purpose is to be used in full site editing.

I'm guessing this is mostly not having to disable all the "unwanted" Nav block features right?

Yes, there's that. The navigation block can also be cleaned up because there are a few places where the nav editor adds conditional logic to the block. The block styles are much much simpler.

I'm confused. Are you suggesting a Menu Item blocks as children of the Navigation block?

I'm suggesting having no parent block. The navigation block in the navigation editor at the moment is entirely featureless and it adds extra UI complexity in terms of being another thing a user can select.

Only if we don't use the Navigation block and utilise this new Menu block instead. Otherwise we'll have to make sure Nav block changes work for the Menu block.

I don't fully understand this, could you go into more detail?

What happens if we decide to allow for block based themes to opt into block based rendering of menus. Wouldn't Themers now need to write styles for the Menu block as well as the Navigation block? It's another area of inconsistency and complexity to be considered.

The menu block would only ever render using the walker system, outputting backwards compatible HTML. Themes already support this. Additionally themes could opt in to navigation block support, which is where a themer can add additional styles. When this is detected, the editor can offer the user the option to convert from a menu block to a fully featured navigation block. But one advantage here over the current implementation is that the user can opt to keep their menu the same.

@draganescu
Copy link
Contributor

I like this a lot. I think the code duplication is a minor problem in this context. As the navigation block evolves more and more towards a full fledged mega menu builder, the code duplication will be less that the code abstraction we would otherwise need on top of the navigation block to keep if from doing unsupported things or breaking.

Before we move on, what are we losing by not continuing with the navigation block as the infrastructure to edit classic menus?

@talldan
Copy link
Contributor Author

talldan commented Sep 7, 2021

Before we move on, what are we losing by not continuing with the navigation block as the infrastructure to edit classic menus?

I think the main things are:

  • Portability of the block needs to be addressed
  • Some duplication of effort - any features implemented in the navigation block that are relevant for the navigation editor will have to be duplicated. Also the reverse of that.

@gwwar
Copy link
Contributor

gwwar commented Sep 21, 2021

I really like the idea here of decoupling the blocks because there are different concerns and workflows that the navigation editor has that the other block editors do not. Trying to share code for different concerns tends to constrain how quickly we iterate (since we're adding complexity) and adds fragility as contributors are not aware of the use cases we need to handle (or when we sometimes forget to test new features in all environments).

One really simple example is attempting to add basic transforms in #34978. Because the blocks are reused by both editors, we end up needing to work around what is the block's source of truth. Navigation supports one set of transforms and the other editors need a different set. Getting this to work currently is a bit of a hack. Edit: it looks like this appears to work without the hook since we don't register the other block types. The transform definition not being literally what we see in the nav editor though is still confusing.

Overall:

Navigation Editor

  • Needs to handle serialization to and from menu items
  • Probably will want a bigger emphasis of link variations or other UX choices to better match the older menu building UX
  • Does not support newer link features (that older menu items would break on), or newer blocks.

Post/Site/Widget Editor

  • Does not need to handle serialization to and from menu items
  • May need to bootstrap a navigation block from an existing menu (but this can be done as a one-way transform).
  • Supports newer navigation items like search, logos, etc
  • May want to diverge UX to support simpler and more modern flows. Maybe different patterns for mega menu building.

@Mamaduka
Copy link
Member

I think @gwwar has a good point.

In the Navigation screen, we have to have feature parity with the previous one. The Navigation block, on the other hand, doesn't need to be constrained by this.

So if the decoupling of blocks will let us iterate much faster, that's a big pro for me right now. Of course, we can always come back and start thinking about reusing components once both features are more stable.

@spacedmonkey
Copy link
Member

+1 For having two blocks here. I think trying to keep the existing block generic enough to work in both places, is just going to slow both projects down ( full site editing / navigation editor ). I also agree, saddling the nav block with all the backwards compatibility constraints doesn't benefit the nav block.

The dedicated blocks have more scope for supporting backwards compatibility (especially around extensibility).

Consider my comment here. Handling custom fields, meta boxes, filter and actions, will require a lot of complex code that is only relevant in the navigation editor. A dedicated block would help with this.

*/
function register_block_edit_navigation_menu_item() {
$post_types = get_post_types( array( 'show_in_nav_menus' => true ), 'objects' );
$taxonomies = get_taxonomies( array( 'show_in_nav_menus' => true ), 'objects' );
Copy link
Member

Choose a reason for hiding this comment

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

At some point I would love to load this data via the REST API.

@draganescu
Copy link
Contributor

While I was initially a big fan of having a legacy menu block, I started to wonder if this PR indicates that we've strayed too far from the original goals of the project.

Looking at our Navigation Editor tracking issue, which is currently prioritized around removing the experimental label from the project, this PR is very good at achieving that. Nevertheless, while this would help with our short term priority, it wouldn't help with our long term aims for this project.

In fact, the exploration in this PR is a prime example of why we're trying to use the navigation block in the navigation editor: to figure out, via a real world implementation, the friction points and shortcomings of: 

  • the navigation block itself, 
  • the block API and 
  • the block editor APIs in dealing with classic WordPress data, in this case menu data.

If making a classic menu block is the only viable solution to the above, it means we've hit quite a lot of these shortcomings, so kudos everyone, now let's improve them.

Primarily the new block editors (widgets and navigation) started as refinement processes to power:

  • increasing classic and block theme compatibility and interoperation (theme switching),
  • offering a new path for block adoption in classic themes,
  • improving the block API and block interface to work with reusable and structured data,
  • improving the block editor APIs for working with blocks in isolation.

This approach here may unblock, and potentially speed up, the development of the navigation editor, but it has a few shortcomings:

  1. It hides from us what the navigation block still is unable to achieve in terms of managing structured tree data editing and rendering, and also the well tested and highly dependable ways in which menus currently work in WordPress.
  2. It leads to functional duplication, since in both classic and block themes menus are first and foremost the same thing: nested trees of links, so both blocks will reimplement the same kind of functionality.
  3. It adds to WordPress one new special case feature, a sort of transition block, to maintain for posterity.

In conclusion, I fear if we pursue this path we’ll lose insights for improving the navigation block and the block platform as a whole, plus introduce a host of duplicate efforts. I believe we're looking to build the best block based experience possible in WordPress and this needs to push the boundaries of the platform in new and exploratory environments that only projects like the navigation editor can create. 

The development speed gains from this exploration are not worth the trade off — so I suggest let’s close this out and focus on resolving  the limitations of the navigation block and of the block platform.

@getdave getdave mentioned this pull request Sep 29, 2021
62 tasks
@mtias
Copy link
Member

mtias commented Sep 29, 2021

First, thank you for doing this exploration and writing out all the perils you've identified. I wanted to chime in here because I've been against a separate block for a long time and I still don't think it's a good solution but I have a clearer perspective of where the mismatch is coming from. Or, to be more precisely, I think if we break too early we miss on a series of important architectural considerations that we need to address in the navigation block through and through.

For example:

  • How does theme switching work for the block-navigation-in-a-template case?
  • Can a menu be reused in different places but customized differently? What does that mean in block themes?
  • How are the server-side APIs structured for interacting with the contents of a menu (automatically adding pages, for example)? See Navigation: Add an API to retrieve content-only inner blocks #30674 which is a primitive description of the same problem.
  • Do block themes support automated import / display of menus and how should that behave?

In answering these I think it'd become more apparent that the characterization and the perception of a classic navigation block being intrinsically different is a bit off:

  • The navigation block got painted in the light of its most bombastic use (mega menus with mixture of presentation and IA mapping) and that was a departure that A Lighter Navigation Block Experience #34041 attempts to reign in.
  • The different use case of this proposal largely overlaps with what the navigation block needs to accomplish in its most common incarnation if we describe it properly anyways.

@talldan's comment about considering not having a "menu" block at all I think comes from perceiving part of the problem but describing it differently. I agree we don't need it for the classic case, but I'd also argue we don't need it for the block case when it comes to representing a menu structure in most situations. Let me articulate that better.

The current parent wrapper of the navigation block — <!-- wp:navigation --> — is taking on the duty of handling most of the customization aspects of the block, so it represents a template concern while its contents are mostly handling the order and relationship of menu items. Conceptually, the classic navigation editor concerns itself only with the inner contents and not with the template that renders it. We should be addressing that for the navigation block case as it would also help solve some of the pending questions!

The container block should be opaque to its children when looked at from the perspective of the template that holds it. We just need to treat the inner contents as what gets saved separately and accessed independently of the navigation block container that wants to customize its look.

In other words, we need to do the conceptual shift from:

<!-- wp:navigation  -->
  ...
<!-- /wp:navigation -->

To this for most common scenarios:

<!-- wp:navigation { id: "menu" } /-->

The classic navigation screen concerns itself with the ... when those are just menu links (not expanded html) in the same way the lighter navigation approach is defaulting to adding link items if there are no other blocks present. The conceptual overlap there is virtually the same and it'd be a shame to lose that.

The template that includes a menu completely eliminates the need for a "location" because that becomes an implicit aspect of use, but it does absorb configuration aspects that are decoupled from the menu structure. For example:

<!-- wp:navigation {"id":"menu","isResponsive":true,"fontSize":"medium"} /-->

Can be used in a header template while a footer can avoid the responsive collapse and use a different font-size. The menu contents are shared.

The inner contents could be shaped in a way that is not purely relational data driven (like a mega menu) in which case reuse is mostly a matter of template part reuse when it fits.


I think we need to look at a next step of treating wp:navigation as an implicit template part (or different CPT, doesn't matter conceptually) first and foremost. I think some concerns will dissipate then. A dedicated screen would just be the equivalent of an isolated template part that discards the template customization to focus on the inner content arrangement. The transposition between current data and block-represented data is a much more analogous enterprise if you discard the parent for purely link-based navigations.

It's still possible that once we do these initial steps the difference that will surface is between purely relational data (link items) and menus that mix some aspects of presentation (mega menus or menus with a site logo in between items). That difference would exist in a block theme context as well, so it's not a classic vs new distinction.

@talldan
Copy link
Contributor Author

talldan commented Oct 1, 2021

I like the idea of a separation between the data part of a menu and the visual representation as part of making the navigation block reusable. It allows us to draw parallels between the full site editing use of the navigation block and the use in the navigation editor, which become very similar. I do think that could help improve the model that we're using.

The tough part seems to be the backwards compatibility aspect of this model.

A question is if the navigation block should be able to read and write classic menus or whether the classic menu should be read once and then saved from that point on as a navigation block (essentially migrating data). The first way is similar to what we do now and means maintaining a lot of technical complexity, something that has been challenging. The second is cleaner, but perhaps harder to maintain back compat and also means for users switching back to the classic menu editor the story becomes interesting. The classic menu editor could probably be made to read from navigation block menus without too much effort, there's already code to do the conversion, so there are options.

@mtias
Copy link
Member

mtias commented Oct 1, 2021

I think two way sync compatibility would be unreasonable to maintain, as you say, and it's not a pattern we have used elsewhere before. For post content, once you go with blocks going back is not warranted to have consistent results; same has been the case for widgets. Consolidating how we store block related data is a good benefit for streamlining operations. The thing with navigation is that with this system we do have more options so we can offer more paths. For example:

  • When a user "migrates" to navigation as blocks:
    • If blocks used are all link-items rendering compatibility could be 100%. Even if stored in template parts we can map the block tree to a wp_nav_menu renderer transparently without much issue.
    • If more blocks are to be used (like social icons, search, etc) a classic theme could choose to opt in by letting the block engine do the rendering (pseudo: do_blocks( 'navigation' )). This aligns well with the way the navigation blocks inserts links by default unless more blocks are present.
  • A block editor using the "focused template part" mode for navigation would then be fine for both block and classic themes as an interface and storage.
    • On block themes the template part container would obviously handle visual display and customization while on classic or focused modes it would just represent the nav structure. Link items in this context would need to support both enabling and disabling style supports. One parallel here to consider is the "disable theme styles" we have in preferences. As block style attributes are captured in supports we can easily toggle them on or off based on what we need to display.

@getdave
Copy link
Contributor

getdave commented Oct 1, 2021

A question is if the navigation block should be able to read and write classic menus or whether the classic menu should be read once and then saved from that point on as a navigation block (essentially migrating data)

I'd say that once the user opts into blocks for navigation other than core/navigation-link then we make it clear (via UI prompt) that this is a lossy operation (ie: there is no two-way sync maintained). It's a one-time read and then we write to blocks from then onward.

However, if the navigation is all "simple" (just links and no other blocks) we can store as either blocks or menu items. This measure of backwards compatibility will allow the navigation editor to continue to fulfil a role as a nice "onboarding ramp" for Full Site Editing whilst that project is maturing.

In terms of data storage and how we handle transforming the different data types...I wonder if we could look to normalize towards having a single unified "schema" for navigations.

In this world we could represent the structure of a navigation in some kind of universal format (JSON or even HTML + block grammar). Then we could have different means of persisting that data:

  1. Persist to blocks (via Navigation block).
  2. Persist to Menu CPTs (via Navigation Editor).

This would mean we'd need to create some kind of transform layer (@wordpress/menus?) that can accept either blocks or classic menu data and convert it into the unified schema.

That way we'd have a Navigation block that would only need to be aware of the universal schema and wouldn't need to care / know about classic Menu data structures. This would also nicely reduce duplication of the existing code for transforming menu items -> blocks (and the inverse) which currently lives in both the editor and the block.

@draganescu
Copy link
Contributor

Consolidating how we store block related data is a good benefit for streamlining operations.

Migrating to blocks could be a one way path, but IMO only when the user switches from a classic theme to a block theme.

I don't know if automatic upgrades, consolidating menu storage as block storage across theme types, are a way to go, even if the nested link blocks block fomat could be packaged to be perfectly digestible by any Walker. People may depend on the current DB menu storage, split across six tables :) I think the schema or similar approach proposed by @getdave above would be the way to find the common ground format between the JSON we get from the API and the html we get from the block storage.

@spacedmonkey
Copy link
Member

I think that, enabling blocks in navigation, will likely end up being a opt-in feature, via a theme supports. It is unlikely that existing themes will allow blocks, like search to be put into menus, as it the css / styling would not likely "just work". So we have to consider backwards compatibility here.

Think about these user stories.

  1. Developers of theme, does not opt in to blocks in navigation. In this case, the data will HAVE to be saved in the old format ( menu item CPT ). The user will not be able to render the blocks correctly, so let's not even try.
  2. Developers of theme, does opt in to blocks in navigation. But only has navigation links in the menu and no special block like social. For best compatibility, this should be saved as a blocks ( in menu description see Navigation Block: save data to a custom post type #34612 (comment) for more details ) and be saved in the old format ( menu item CPT ). This will provide the most compatibility with existing themes and will make theme switching very easy.
  3. Developers of theme, does opt in to blocks in navigation and a user saves a mixture of blocks. This is a lossy operation. This means if the user changes themes, then we can't render anything. We can add warning in wp admin, when user tries to switch theme / in menu screens.

Again, I strongly recommend using term description to store block data, see #34612 (comment) for more details.

@talldan
Copy link
Contributor Author

talldan commented Oct 4, 2021

I think that, enabling blocks in navigation, will likely end up being a opt-in feature, via a theme supports. It is unlikely that existing themes will allow blocks, like search to be put into menus, as it the css / styling would not likely "just work". So we have to consider backwards compatibility here.

I think for non-block themes it would definitely be bad to impose block html being rendered on the frontend, I agree with that, it would very likely become a backwards compatibility mess. At the same time, the navigation editor already depends heavily on the idea of translating menu items to blocks and back again. It could be possible to make a walker to work with link blocks by converting those blocks back into the menu item format before the HTML is output, and that would have better backward compatibility results.

What would be difficult is catering for all the extensibility that can happen with menu items, things like custom fields.

I strongly recommend using term description to store block data, see #34612 (comment) for more details.

Something that I hadn't considered at all, thanks for mentioning it. Let's continue the discussion on #34612 about that.

@draganescu
Copy link
Contributor

the navigation editor already depends heavily on the idea of translating menu items to blocks

Just to crosslink my comment #34612 (comment) where I suggest we move this transforming concern out of the block.

@tellthemachines
Copy link
Contributor

On block themes the template part container would obviously handle visual display and customization

To make sure we're all talking about the same thing here 😅 , would the "template part container" in this case be the Navigation block? Asking because we've been discussing these ideas here and are unclear on that point.

Link items in this context would need to support both enabling and disabling style supports

Wouldn't the current setup where the Navigation contents inherit context from their parent work? The link items themselves shouldn't have any style controls, but if styling is present in their container they use it.

@talldan
Copy link
Contributor Author

talldan commented Oct 5, 2021

To make sure we're all talking about the same thing here 😅 , would the "template part container" in this case be the Navigation block

@tellthemachines My understanding is that would be the case, but I don't really see the connection to that comment of mine.

@tellthemachines
Copy link
Contributor

@tellthemachines My understanding is that would be the case, but I don't really see the connection to that comment of mine.

This discussion is becoming super confusing 😅

You were replying to my question on whether the nav editor would be using the Navigation block or just its contents. Because the Nav block has all the styling options etc, there really isn't anything in the block itself that we need to use in the editor, at least data-wise.

I may not have been fully clear with my question above. I should have asked instead:

On block themes the template part container would obviously handle visual display and customization while on classic or focused modes it would just represent the nav structure

Is the template part container used in block themes the same as the one used in "classic or focused modes"?

Because I took <!-- wp:navigation { id: "menu" } /--> to mean the menu, as contents of the nav block, is coming in from somewhere else (a custom post type), and we can grab that menu and use it in the nav editor, independently of the nav block.

@getdave
Copy link
Contributor

getdave commented Oct 13, 2021

Because I took <!-- wp:navigation { id: "menu" } /--> to mean the menu, as contents of the nav block, is coming in from somewhere else (a custom post type), and we can grab that menu and use it in the nav editor, independently of the nav block.

Yes this is what I currently believe.

  • core/navigation - wrapper concerned with presentational purposes. Akin to Template Part.
  • core/navigation-link (and friends!) - inner blocks representing the structure/data of a "navigation".

The implication for the Nav Editor is that it no longer utilises the core/navigation block at all. Rather it acts like an Isolated Template Part editor and contains only the child blocks of the Navigation.

This is very similar to the existing classic Menus screen which is only concerned with the structure/data of the navigation and not the presentation of the Navigation.

@talldan talldan closed this Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants