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

Try using a template part in the navigation block #35418

Closed
wants to merge 10 commits into from

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Oct 7, 2021

Description

Closes #34612

This is a very unrefined PR that adds template part storage to the nav block.

Current problems:

  • Navigation block template parts show up in the template part block and vice versa
  • The navigation block now has a multi stage creation process that could use some refinement
  • A lot of code is borrowed from the template part block, so the nav block looks a lot like a template part
  • It'd be good to make sure existing (non-template part) navigation blocks can be saved in template parts.
  • Navigation editor no longer works

How has this been tested?

  1. Open the site editor (avoid testing in the post editor because it doesn't seem to save template part entities)
  2. Create a nav block

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

@talldan talldan added the [Block] Navigation Affects the Navigation Block label Oct 7, 2021
@talldan talldan self-assigned this Oct 7, 2021
@github-actions
Copy link

github-actions bot commented Oct 7, 2021

Size Change: +4.57 kB (0%)

Total Size: 1.07 MB

Filename Size Change
build/block-directory/index.min.js 6.2 kB +3 B (0%)
build/block-editor/index.min.js 134 kB -293 B (0%)
build/block-editor/style-rtl.css 13.9 kB -12 B (0%)
build/block-editor/style.css 13.9 kB -11 B (0%)
build/block-library/blocks/cover/style-rtl.css 1.17 kB -56 B (-5%)
build/block-library/blocks/cover/style.css 1.17 kB -56 B (-5%)
build/block-library/blocks/image/editor-rtl.css 731 B +3 B (0%)
build/block-library/blocks/image/editor.css 730 B +2 B (0%)
build/block-library/blocks/image/style-rtl.css 502 B +20 B (+4%)
build/block-library/blocks/image/style.css 505 B +18 B (+4%)
build/block-library/blocks/media-text/style-rtl.css 493 B +5 B (+1%)
build/block-library/blocks/media-text/style.css 490 B +5 B (+1%)
build/block-library/blocks/navigation/editor-rtl.css 1.72 kB -2 B (0%)
build/block-library/blocks/navigation/editor.css 1.72 kB -1 B (0%)
build/block-library/blocks/post-featured-image/editor-rtl.css 396 B -2 B (-1%)
build/block-library/blocks/post-featured-image/editor.css 397 B -1 B (0%)
build/block-library/blocks/post-featured-image/style-rtl.css 156 B +10 B (+7%) 🔍
build/block-library/blocks/post-featured-image/style.css 156 B +10 B (+7%) 🔍
build/block-library/blocks/site-logo/editor-rtl.css 769 B +307 B (+66%) 🆘
build/block-library/blocks/site-logo/editor.css 769 B +305 B (+66%) 🆘
build/block-library/blocks/site-logo/style-rtl.css 165 B +12 B (+8%) 🔍
build/block-library/blocks/site-logo/style.css 165 B +12 B (+8%) 🔍
build/block-library/editor-rtl.css 9.89 kB +176 B (+2%)
build/block-library/editor.css 9.89 kB +175 B (+2%)
build/block-library/index.min.js 148 kB +1.91 kB (+1%)
build/block-library/reset-rtl.css 474 B -62 B (-12%) 👏
build/block-library/reset.css 474 B -62 B (-12%) 👏
build/block-library/style-rtl.css 10.4 kB -73 B (-1%)
build/block-library/style.css 10.4 kB -71 B (-1%)
build/blocks/index.min.js 45.7 kB +13 B (0%)
build/components/index.min.js 217 kB +3.29 kB (+2%)
build/components/style-rtl.css 15.3 kB -652 B (-4%)
build/components/style.css 15.3 kB -647 B (-4%)
build/dom/index.min.js 4.47 kB +15 B (0%)
build/edit-post/index.min.js 29.2 kB +6 B (0%)
build/edit-post/style-rtl.css 7.2 kB +7 B (0%)
build/edit-post/style.css 7.19 kB +7 B (0%)
build/edit-site/index.min.js 29.5 kB +264 B (+1%)
build/edit-site/style-rtl.css 5.5 kB +28 B (+1%)
build/edit-site/style.css 5.5 kB +27 B (0%)
build/reusable-blocks/index.min.js 2.19 kB -90 B (-4%)
build/server-side-render/index.min.js 1.52 kB +25 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.21 kB
build/autop/index.min.js 2.08 kB
build/blob/index.min.js 459 B
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
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 206 B
build/block-library/blocks/columns/editor.css 205 B
build/block-library/blocks/columns/style-rtl.css 497 B
build/block-library/blocks/columns/style.css 496 B
build/block-library/blocks/cover/editor-rtl.css 666 B
build/block-library/blocks/cover/editor.css 670 B
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 977 B
build/block-library/blocks/gallery/editor.css 982 B
build/block-library/blocks/gallery/style-rtl.css 1.6 kB
build/block-library/blocks/gallery/style.css 1.59 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 78 B
build/block-library/blocks/group/theme.css 78 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 332 B
build/block-library/blocks/html/editor.css 333 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/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 568 B
build/block-library/blocks/navigation-link/editor.css 570 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-submenu/editor-rtl.css 300 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/style-rtl.css 195 B
build/block-library/blocks/navigation-submenu/style.css 195 B
build/block-library/blocks/navigation-submenu/view.min.js 343 B
build/block-library/blocks/navigation/style-rtl.css 1.62 kB
build/block-library/blocks/navigation/style.css 1.61 kB
build/block-library/blocks/navigation/view.min.js 2.74 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 377 B
build/block-library/blocks/page-list/editor.css 377 B
build/block-library/blocks/page-list/style-rtl.css 198 B
build/block-library/blocks/page-list/style.css 198 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 273 B
build/block-library/blocks/paragraph/style.css 273 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-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-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 391 B
build/block-library/blocks/post-template/style.css 392 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 262 B
build/block-library/blocks/query-pagination/editor.css 255 B
build/block-library/blocks/query-pagination/style-rtl.css 234 B
build/block-library/blocks/query-pagination/style.css 231 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-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.3 kB
build/block-library/blocks/social-links/style.css 1.3 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 853 B
build/block-library/common.css 849 B
build/block-library/theme-rtl.css 665 B
build/block-library/theme.css 669 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/compose/index.min.js 10.3 kB
build/core-data/index.min.js 12.4 kB
build/customize-widgets/index.min.js 11.2 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/edit-navigation/index.min.js 15.3 kB
build/edit-navigation/style-rtl.css 3.74 kB
build/edit-navigation/style.css 3.74 kB
build/edit-post/classic-rtl.css 492 B
build/edit-post/classic.css 494 B
build/edit-widgets/index.min.js 15.7 kB
build/edit-widgets/style-rtl.css 4.1 kB
build/edit-widgets/style.css 4.1 kB
build/editor/index.min.js 37.5 kB
build/editor/style-rtl.css 3.76 kB
build/editor/style.css 3.75 kB
build/element/index.min.js 3.17 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 5.93 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 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.72 kB
build/keycodes/index.min.js 1.3 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.92 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/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.6 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 7.11 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

"slug": {
"type": "string"
},
"theme": {
Copy link
Member

Choose a reason for hiding this comment

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

Why would we need to keep a reference to "theme"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR is very copy/paste from the template part block at the moment. I'll look at removing this 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yes, makes sense :D I think we should be as lean as possible when it comes to borrowing from the template part structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yet to find an easy way to achieve this, it seems as though a template part can only be fetched when it has a theme.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably remove that limitation, it's coupling template parts too much with themes, and it should be entirely feasible (maybe even the norm down the road) to have template parts that are not associated with any specific theme at all.

@mtias mtias added the [Status] In Progress Tracking issues with work in progress label Oct 7, 2021
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

So this is very cool. Thanks so much for exploring it.

I gave it a test drive and the link to the video below has some commentary which explains what I found.

🎥 Test Drive Video (it was too large for Github!)

TL;DR

  • The basic process works well - the "data" (ie: the items in your Nav) are saved to template part without the wrapping core/navigation block.
  • You can reuse the same navigation in different places on the same site and updates in one location will be reflected in another.
  • You can access the navigation in the Isolated Template Part Editor and you will see only the "inner" navigation block items (no Navigation block).

Things we will need to address (I know you are aware of many of these):

  • When in Isolated Template Part editor you cannot add child navigation-type blocks (e.g. core/navigation-link because there is no wrapping parent Navigation block and these blocks are set to only appear if they are a child of that block.
  • I couldn't seem to access the nav template part on another Theme. This might be a limitation of the Template Parts themselves at the moment. The good thing is we will inherit much of the improvements if we reuse some/all of the Template Part mechanics.
  • UI/UX - this obviously needs refinement to mask the "Template Part" specific UI and tailor towards Navigation. I think the user shouldn't necessarily need to know we are using Template Parts.

Overall however this is a great start and everyone should experiment with it to get a sense of where it could take us.

@noisysocks
Copy link
Member

Very cool! Surprising to see it work so well at this early stage.

  • When in Isolated Template Part editor you cannot add child navigation-type blocks (e.g. core/navigation-link because there is no wrapping parent Navigation block and these blocks are set to only appear if they are a child of that block.

This exposes an interesting question. Do we want the settings which are stored on the core/navigation block to be shared across templates? For example, if the navigation has a horizontal layout then would we want it to be appear horizontal in all other templates where it's used?

If yes, then I think the solution here is to store the core/navigation block inside the template part CPT. With this approach there isn't really a difference between a navigation and a template part. Perhaps core/navigation can be hidden from the inserter and "Navigation" is just a variation of the core/template-part block that contains a core/navigation.

If not, then I think the solution here is to use a different CPT for navigation so that it doesn't appear in the template part editor. We can build a custom template part editor for navigation which provides the wrapping core/navigation block.

@talldan
Copy link
Contributor Author

talldan commented Oct 8, 2021

If not, then I think the solution here is to use a different CPT for navigation so that it doesn't appear in the template part editor. We can build a custom template part editor for navigation which provides the wrapping core/navigation block.

The goal right now is to keep the inner blocks separate from the settings as you mention here, the idea being that menu data can be reused but given a different appearance depending on the location.

The navigation block also has the inverse problem that you shouldn't be able to choose non navigation block template parts from the placeholder. Either the template part could have a way to identify the block type it's associated with, or as you say a separate CPT could be used. Some previous discussion here - #34612 (comment).

@getdave
Copy link
Contributor

getdave commented Oct 8, 2021

Good questions @noisysocks!

Do we want the settings which are stored on the core/navigation block to be shared across templates? For example, if the navigation has a horizontal layout then would we want it to be appear horizontal in all other templates where it's used?

I think we wouldn't want this. For example if I have a primary menu I want to display that in the header and the footer but in totally different ways.

For example, the header might be horizontal with dropdown menus. The footer might be horizontal but with all submenus shown vertical below each item (like a "fat footer"). The data should be the same but the styling/layout might need to be completely different.

IMHO the layout/appearance rules applied to the Nav block itself should be specific to the insertion location in the Template where it is added. That means not storing it alongside the inner blocks within the post entity. The keeps styling/layout (Nav block as "Template Part") separate from "data" (the navigation items as inner blocks).

This also means we could reuse "just the data" in the the Navigation Editor without having to undo/disable a load of layout/styling config coming from the Nav block itself.

@talldan Does the above sound like what you are thinking too?

@adamziel
Copy link
Contributor

adamziel commented Oct 8, 2021

Great work @talldan, keep it coming!

Do we want the settings which are stored on the core/navigation block to be shared across templates? For example, if the navigation has a horizontal layout then would we want it to be appear horizontal in all other templates where it's used?

I actually like being able to have it vertical on one page and horizontal on another. If I want to share that, nothing stops me from creating a new wrapping template part just for the Navigation block.

This also means we could reuse "just the data" in the the Navigation Editor without having to undo/disable a load of layout/styling config coming from the Nav block itself.

I'm all for that, but I can't think of an approach that doesn't involve duplicating a lot of the code from the nav block. Exporting NavigationInnerBlocks from block-library would help, but seems controversial. Any ideas?

@getdave
Copy link
Contributor

getdave commented Oct 8, 2021

I'm all for that, but I can't think of an approach that doesn't involve duplicating a lot of the code from the nav block

I assume you mean CSS and also some of the JS utilities that come baked into the Nav block?

I starting to think perhaps having the CSS of the Nav block decoupled from the editor is a good thing. If the inner blocks' styles are similarly decoupled from the parent Nav block then this should mean they will "still work" with just a little custom CSS in the Nav Editor.

In terms of utilities I assume we could look to share a lot of this via some kind of shared package such as @wordpress/menus?

Just initial thoughts - I haven't dived into any of this yet.

@adamziel
Copy link
Contributor

adamziel commented Oct 8, 2021

I'm all for that, but I can't think of an approach that doesn't involve duplicating a lot of the code from the nav block

I assume you mean CSS and also some of the JS utilities that come baked into the Nav block?

I meant all the JS from the Navigation block and NavigationInnerBlocks component, there is a fair bit of work done there.

I starting to think perhaps having the CSS of the Nav block decoupled from the editor is a good thing. If the inner blocks' styles are similarly decoupled from the parent Nav block then this should mean they will "still work" with just a little custom CSS in the Nav Editor.

👍

In terms of utilities I assume we could look to share a lot of this via some kind of shared package such as @wordpress/menus?

It would work, although @wordpress/block-library would have to depend on it – if that's not an issue then we may be good.

@draganescu
Copy link
Contributor

Aww 😍 we're moving to the place!

I think the solution here is to store the core/navigation block inside the template part CPT

We'd be back to having to configure the block from outside settings and we'd also step away from the "reuse data" idea.

I think we should just not make it a "template part" but a "navigation" .... part, case in which we can tweak the behavior a lot: auto wrap in a nav block, correct what blocks are available to insert and so on.

@talldan
Copy link
Contributor Author

talldan commented Oct 11, 2021

@talldan Does the above sound like what you are thinking too?

I think you might have missed the comment I made just before yours commenting on that question, might have been a simultaneous post - #35418 (comment)

It seems like there are a lot of good advantages from not saving the navigation block wrapper in the template part.

It does mean wherever that template part is edited in isolation (navigation editor, template part editor, and potentially the template part focus mode) has some complexity, similar to what we've already been experiencing in the navigation editor.

If left as it is now, we'd have a situation where only the inner blocks can be edited. This breaks some concepts like how the Navigation Link has a 'parent' property. The 'direct insert' behavior that was just implemented no longer works and would need to be replicated.

The other option is to include a navigation block as a locked wrapper in these isolated editors. This fixes the things that were just mentioned, but also has some drawbacks. All the styling options on the navigation block need to be disabled (except for in template part focus mode, where this might still be relevant). Another conundrum is that the navigation block will likely have an option to switch to a different template part, but does that make sense when editing a single template part already? It would seem like that's the job of the editor.

@tellthemachines
Copy link
Contributor

This looks very promising 🚀

I think conceptually at least it makes sense for Navigation to be its own custom post type, because template parts by definition are freeform, all-blocks-allowed areas, whereas what we want here is a block that is constrained to accept only certain types of blocks, and has a bunch of custom behaviour (direct insert and so forth).

It would also mean we could have a "Navigation" section in the Appearance menu, which would make it easier to manage menus in the admin. As it is, we need to go looking for them in the Template Parts page. And perhaps, when opening a Navigation CPT from the Appearance menu, we could show it in a navigation editor type interface 😄

Would creating a new CPT imply a lot of duplication of logic that we already have in the Template Part?

@talldan
Copy link
Contributor Author

talldan commented Oct 11, 2021

Would creating a new CPT imply a lot of duplication of logic that we already have in the Template Part?

That's definitely the concern.

@adamziel
Copy link
Contributor

adamziel commented Oct 12, 2021

Would creating a new CPT imply a lot of duplication of logic that we already have in the Template Part?

How does a new CPT compare to having template parts "variations" with limited capabilities e.g. configurable allowedBlocks? I'm thinking of "general" template part and "navigation" template part. Are there any other use cases for that? A "post" template part perhaps?

@mtias
Copy link
Member

mtias commented Oct 12, 2021

Thank you very much for diving into this exploration. It seems the most important decision is whether the template part abstraction is a good enough fit, and provides enough value that it offsets the extra work needed to accommodate the specificities and differences that will inevitably arise between navigation and general purpose template parts.

Some of these differences might be pointing at shortcomings of the template part structure, which we should improve (for example, how tied things are to a theme when it comes to template parts, which I think is shortsighted on our design). Others might end up with so many exclusions (like not wanting it show in the "mosaic view" of template parts, or the template part selectors, etc) that we'd end up spreading through the codebase exclusions for navigation and a maintenance burden.

We should probably list those considerations, both concrete benefits we get and negatives we incur when using wp_template_part vs a new wp_navigation modelled after it.

@talldan
Copy link
Contributor Author

talldan commented Oct 13, 2021

How does a new CPT compare to having template parts "variations" with limited capabilities e.g. configurable allowedBlocks? I'm thinking of "general" template part and "navigation" template part. Are there any other use cases for that? A "post" template part perhaps?

Template parts do already use variations for semantic areas (Header, Footer).

@talldan
Copy link
Contributor Author

talldan commented Oct 14, 2021

We should probably list those considerations, both concrete benefits we get and negatives we incur when using wp_template_part vs a new wp_navigation modelled after it.

Yep, agreed. Here's an attempt to define some of the problems we're currently facing

  • A way to associate a block type with a template part might be needed. This would prevent other non-navigation template parts being visible in the navigation block (and also the opposite).
  • Navigation block template parts are also editable from wp-admin and the focus mode, but many of the inner block types of the navigation block have a 'parent' defined and can only be inserted within a particular parent. Editing in isolation will likely need a special case to either wrap blocks with a navigation block or for the editor to imply the inner blocks are within a navigation block without one actually being present.
  • Template parts seem to be strongly associated with a theme and a template part created in one theme is unavailable in another. This isn't a requirement for the navigation block. Is it a requirement for custom template parts, or is this something that needs to be fixed?
  • Template parts have an 'area' that defines the semantic region of a page. From what I can tell this doesn't really fit in with the way a navigation block might have a location. For a template part, the area defines a semantic region of a site (e.g. header). For a navigation block, the location is defined by where it is rendered. A single navigation block template part might also be rendered in more than one place, so a single area doesn't make sense. It might be interesting to look at a way for the navigation block to inherit a location from a parent template part's area. The parent area could also be useful for suggesting patterns or styles similar to how a template part does.
  • Also worth mentioning the current use cases for a navigation block 'location'. Theme switching is one, where a classic menu location might have to be written to a template part. The API described in Navigation: Add an API to retrieve content-only inner blocks #30674 might be another if a user wants to build a sitemap based on their main menu.

@mtias
Copy link
Member

mtias commented Oct 15, 2021

This would prevent other non-navigation template parts being visible in the navigation block (and also the opposite)

This would be relatively straightforward from the perspective of the navigation block, and would follow how specific areas can be swapped. Here you only see "headers" for example and not other template parts:

image

I think the challenge is more about the cases where all template parts are shown, and whether we need to exclude navigation from those. That's what could start to get things a bit convoluted in my view.

Template parts seem to be strongly associated with a theme and a template part created in one theme is unavailable in another. This isn't a requirement for the navigation block. Is it a requirement for custom template parts, or is this something that needs to be fixed?

The latter. Currently the coupling is overbearing and a self-imposed limitation. I should be able to access template parts I've saved on my site regardless of what theme I'm using. The theme a template part was created in can be useful information but should not be used as a strong filter.

A single navigation block template part might also be rendered in more than one place, so a single area doesn't make sense.

The area in this case would also loosely refer to nav. It's possible to have more than one header in a page, etc.

Navigation block template parts are also editable from wp-admin and the focus mode, but many of the inner block types of the navigation block have a 'parent' defined and can only be inserted within a particular parent.

I think the parent here should be equivalent and somehow present in the isolated template part as a provider. Also somewhat related: #30641.

@carlomanf
Copy link

Template parts seem to be strongly associated with a theme and a template part created in one theme is unavailable in another. This isn't a requirement for the navigation block. Is it a requirement for custom template parts, or is this something that needs to be fixed?

#31971 goes a long way towards fixing this.

@talldan
Copy link
Contributor Author

talldan commented Oct 19, 2021

I've made an alternative PR that uses a wp_navigation post type - #35746

@talldan
Copy link
Contributor Author

talldan commented Oct 19, 2021

I think the challenge is more about the cases where all template parts are shown, and whether we need to exclude navigation from those. That's what could start to get things a bit convoluted in my view.

@mtias Yes, I agree with this, it's definitely where things are getting complicated. If there were a way to solve this a lot of the other problems would also be easier to solve, but I question if it's something that should be solved. Feels like it would be introducing lots of granularity to template parts in order to reuse some of the existing systems like areas. Instead, maybe some of those systems should be abstracted to work more easily with different post types.

As mentioned above, I've made #35746, which naturally provides a separate page for listing menus:
Screenshot 2021-10-19 at 12 17 55 pm

But we still have the same issue that the post editor isn't currently suitable for editing these menus. I guess this is where we see the 'Isolated Editor' being used? Should this listing even be available in the admin?

I should be able to access template parts I've saved on my site regardless of what theme I'm using.

Looks like @adamziel has created a PR to solve this (#35666) 🎉

The area in this case would also loosely refer to nav. It's possible to have more than one header in a page, etc.

Would you be able to elaborate on 'loosely refer to nav'?

I think the parent here should be equivalent and somehow present in the isolated template part as a provider. Also somewhat related: #30641.

Yeah, that would be interesting. In the Navigation (and to some degree Widgets) editors, hardcoded locked templates were used, but that approach wasn't perfect. It feels like the block should instead be assuming the role of the root block list.

@getdave getdave mentioned this pull request Oct 20, 2021
38 tasks
@talldan talldan closed this Oct 29, 2021
@talldan talldan deleted the update/nav-block-to-use-template-parts branch October 29, 2021 07:35
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 [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation Block: save data to a custom post type
8 participants