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: Navigation Menu Sidebar #38600

Closed
wants to merge 3 commits into from
Closed

Try: Navigation Menu Sidebar #38600

wants to merge 3 commits into from

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Feb 8, 2022

Part of #36667.

This is a wide exploration PR which adds a new sidebar to the site and post editor to help with navigation menu management.

⚠️ PLEASE NOTE: A smaller, more targettted PR is now available in #39290 which focuses solely on the Navigation sidebar without addressing wider concerns about the "W" sidebar and global styles.

After

The new navigation sidebar reuses the ListView component and allows users to easily reorder items even when the menu might be hidden (for example in a mobile menu).

CleanShot.2022-02-17.at.09.38.25.mp4

Before

Previously, clicking on the open list view button would open a modal.

Open List View Modal
CleanShot 2022-02-10 at 13 44 02@2x CleanShot 2022-02-10 at 13 43 00@2x

Testing Instructions

  • Verify that in the Site Editor, opening list view now opens the new sidebar as expected
  • We should see a loader when waiting for menu results
  • Verify that in the Widget, Navigation and Post editor that the navigation sidebar is not available.

Notes

Navigation blocks can store their data in many ways:

  1. With a reference to a wp_navigation post id
  2. __unstableLocation such as a "primary" menu. The site editor incorrectly renders an empty placeholder in this case. Note that I won't address that in this PR.
  3. With data set in innerBlocks.
  4. A navigation block may also be populated from a "classic" menu

Wiring up the list view button on the Navigation block toolbar is a package puzzle.

  • Each sidebar is namespaced to a particular editor instance, in case functionality needs to diverge. So for example the action we dispatch to open the sidebar in site editor is:
enableComplementaryArea(
			'core/edit-site',
			'edit-site/navigation-menu'
		); 

while in the post editor it is:

enableComplementaryArea(
			'core/edit-post',
			'edit-post/navigation-menu'
		);
  • block-library should not know about a particular editor instance
  • the toolbar in block-editor should not know about particular blocks

To solve this, I opted to add an unstable slot/fill following the pattern set by __unstableBlockSettingsMenuFirstItem. If folks have a better idea of how to untangle this, I'm happy to hear about suggestions.

@gwwar gwwar self-assigned this Feb 8, 2022
@github-actions
Copy link

github-actions bot commented Feb 8, 2022

Size Change: +1.21 kB (0%)

Total Size: 1.16 MB

Filename Size Change
build/block-editor/index.min.js 144 kB +164 B (0%)
build/block-editor/style-rtl.css 14.9 kB +40 B (0%)
build/block-editor/style.css 14.9 kB +40 B (0%)
build/block-library/index.min.js 167 kB -307 B (0%)
build/core-data/index.min.js 14 kB +23 B (0%)
build/edit-navigation/style-rtl.css 4.05 kB +11 B (0%)
build/edit-navigation/style.css 4.06 kB +12 B (0%)
build/edit-post/style-rtl.css 7.25 kB +10 B (0%)
build/edit-post/style.css 7.25 kB +11 B (0%)
build/edit-site/index.min.js 42.8 kB +928 B (+2%)
build/edit-site/style-rtl.css 7.56 kB +128 B (+2%)
build/edit-site/style.css 7.55 kB +130 B (+2%)
build/edit-widgets/style-rtl.css 4.41 kB +11 B (0%)
build/edit-widgets/style.css 4.4 kB +11 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 993 B
build/admin-manifest/index.min.js 1.24 kB
build/annotations/index.min.js 2.77 kB
build/api-fetch/index.min.js 2.27 kB
build/autop/index.min.js 2.15 kB
build/blob/index.min.js 487 B
build/block-directory/index.min.js 6.49 kB
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 150 B
build/block-library/blocks/audio/editor.css 150 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 445 B
build/block-library/blocks/button/editor.css 445 B
build/block-library/blocks/button/style-rtl.css 560 B
build/block-library/blocks/button/style.css 560 B
build/block-library/blocks/buttons/editor-rtl.css 292 B
build/block-library/blocks/buttons/editor.css 292 B
build/block-library/blocks/buttons/style-rtl.css 275 B
build/block-library/blocks/buttons/style.css 275 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 103 B
build/block-library/blocks/code/style.css 103 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 406 B
build/block-library/blocks/columns/style.css 406 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-template/style-rtl.css 127 B
build/block-library/blocks/comment-template/style.css 127 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-query-loop/editor-rtl.css 95 B
build/block-library/blocks/comments-query-loop/editor.css 95 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/cover/style-rtl.css 1.56 kB
build/block-library/blocks/cover/style.css 1.56 kB
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 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 353 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 965 B
build/block-library/blocks/gallery/editor.css 967 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 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/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 731 B
build/block-library/blocks/image/editor.css 730 B
build/block-library/blocks/image/style-rtl.css 518 B
build/block-library/blocks/image/style.css 523 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 199 B
build/block-library/blocks/latest-posts/editor.css 198 B
build/block-library/blocks/latest-posts/style-rtl.css 447 B
build/block-library/blocks/latest-posts/style.css 446 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 493 B
build/block-library/blocks/media-text/style.css 490 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 649 B
build/block-library/blocks/navigation-link/editor.css 650 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 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/view.min.js 375 B
build/block-library/blocks/navigation/editor-rtl.css 2.03 kB
build/block-library/blocks/navigation/editor.css 2.04 kB
build/block-library/blocks/navigation/style-rtl.css 1.89 kB
build/block-library/blocks/navigation/style.css 1.88 kB
build/block-library/blocks/navigation/view.min.js 2.85 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 363 B
build/block-library/blocks/page-list/editor.css 363 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 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/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/style-rtl.css 446 B
build/block-library/blocks/post-comments-form/style.css 446 B
build/block-library/blocks/post-comments/style-rtl.css 521 B
build/block-library/blocks/post-comments/style.css 521 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 721 B
build/block-library/blocks/post-featured-image/editor.css 721 B
build/block-library/blocks/post-featured-image/style-rtl.css 153 B
build/block-library/blocks/post-featured-image/style.css 153 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 323 B
build/block-library/blocks/post-template/style.css 323 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 80 B
build/block-library/blocks/post-title/style.css 80 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 389 B
build/block-library/blocks/pullquote/style.css 388 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 221 B
build/block-library/blocks/query-pagination/editor.css 211 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/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 201 B
build/block-library/blocks/quote/style.css 201 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 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 397 B
build/block-library/blocks/search/style.css 398 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 233 B
build/block-library/blocks/separator/style.css 233 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 744 B
build/block-library/blocks/site-logo/editor.css 744 B
build/block-library/blocks/site-logo/style-rtl.css 181 B
build/block-library/blocks/site-logo/style.css 181 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 177 B
build/block-library/blocks/social-link/editor.css 177 B
build/block-library/blocks/social-links/editor-rtl.css 674 B
build/block-library/blocks/social-links/editor.css 673 B
build/block-library/blocks/social-links/style-rtl.css 1.37 kB
build/block-library/blocks/social-links/style.css 1.36 kB
build/block-library/blocks/spacer/editor-rtl.css 332 B
build/block-library/blocks/spacer/editor.css 332 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 226 B
build/block-library/blocks/tag-cloud/style.css 227 B
build/block-library/blocks/template-part/editor-rtl.css 235 B
build/block-library/blocks/template-part/editor.css 235 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/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 934 B
build/block-library/common.css 932 B
build/block-library/editor-rtl.css 9.92 kB
build/block-library/editor.css 9.92 kB
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/style-rtl.css 11.4 kB
build/block-library/style.css 11.4 kB
build/block-library/theme-rtl.css 665 B
build/block-library/theme.css 670 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/blocks/index.min.js 46.4 kB
build/components/index.min.js 217 kB
build/components/style-rtl.css 15.6 kB
build/components/style.css 15.6 kB
build/compose/index.min.js 11.2 kB
build/customize-widgets/index.min.js 11.2 kB
build/customize-widgets/style-rtl.css 1.39 kB
build/customize-widgets/style.css 1.39 kB
build/data-controls/index.min.js 663 B
build/data/index.min.js 8.03 kB
build/date/index.min.js 31.9 kB
build/deprecated/index.min.js 518 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.53 kB
build/edit-navigation/index.min.js 16.1 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/edit-post/index.min.js 29.9 kB
build/edit-widgets/index.min.js 16.5 kB
build/editor/index.min.js 38.4 kB
build/editor/style-rtl.css 3.71 kB
build/editor/style.css 3.71 kB
build/element/index.min.js 4.29 kB
build/escape-html/index.min.js 548 B
build/format-library/index.min.js 6.62 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.66 kB
build/html-entities/index.min.js 454 B
build/i18n/index.min.js 3.79 kB
build/is-shallow-equal/index.min.js 535 B
build/keyboard-shortcuts/index.min.js 1.83 kB
build/keycodes/index.min.js 1.41 kB
build/list-reusable-blocks/index.min.js 1.75 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.94 kB
build/notices/index.min.js 957 B
build/nux/index.min.js 2.12 kB
build/nux/style-rtl.css 751 B
build/nux/style.css 749 B
build/plugins/index.min.js 1.98 kB
build/preferences/index.min.js 1.2 kB
build/primitives/index.min.js 949 B
build/priority-queue/index.min.js 611 B
build/react-i18n/index.min.js 704 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.69 kB
build/reusable-blocks/index.min.js 2.24 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 11.1 kB
build/server-side-render/index.min.js 1.61 kB
build/shortcode/index.min.js 1.52 kB
build/token-list/index.min.js 668 B
build/url/index.min.js 1.94 kB
build/viewport/index.min.js 1.08 kB
build/warning/index.min.js 280 B
build/widgets/index.min.js 7.21 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.07 kB

compressed-size-action

@jasmussen
Copy link
Contributor

Very cool. There are some excellent first steps to take in this comment, and your PR and it feels like this one is blazing a trail towards that!

On the side, I think we can explore some of the larger design-related changes with some fresh mockups, I'll get on that, but it's nothing that should block this functionality, so thanks for working on this!

I'm open to early interaction feedback. For example @jasmussen if we added a select dropdown, what would that look like? Should we limit the listing to just the visible menus in the editor (in global blocks) or should we fetch all published menus?

In general I'm always a fan of starting as simple as possible and only add complexity on top of that when we learn that it's necessary. That could look like this:

  • If there's only a single navigation stored, don't show the dropdown at all, just choose that.
  • If there's multiple, default to showing the first one, and surface the dropdown letting you pick the others.

Buttons for creating and managing menus can always come later. But an additional benefit of starting with less is that it lets us figure out how prominent such controls need to be; is such menu management something you do all the time? Or do most sites indeed have a between zero and 2 menus in total?

Something to consider for the future is that case where a site has zero menus, and whether it might make sense to show all the pages of your site and only save it as a CPT if you make a change.

@gwwar
Copy link
Contributor Author

gwwar commented Feb 8, 2022

Something to consider for the future is that case where a site has zero menus, and whether it might make sense to show all the pages of your site and only save it as a CPT if you make a change.

Right, I think there's some interesting navigation menu building we can iterate on in future PRs. For the 0 navigation blocks case initially I might not show the sidebar, or perhaps just note that there are no navigation blocks (and maybe link to a shortcut for inserting one into the document).

If there's only a single navigation stored, don't show the dropdown at all, just choose that.
If there's multiple, default to showing the first one, and surface the dropdown letting you pick the others

Sounds good to me. I suppose we'll also see how useful the navigation menu names are. If we end up using it a lot I might need to rethink how we store data on the client to make the lookup a little less awkward.

One other thought is that folks will likely want to edit classic menus too, but that can also be left as a follow up. I imagine that we don't want to necessarily convert these navigation menus automatically.

@gwwar
Copy link
Contributor Author

gwwar commented Feb 8, 2022

Not feeling great 🤧 so leaving some midday notes for myself:

  • There's a straightforward action we can dispatch to enable a sidebar area (enableComplementaryArea), however these are namespaced to each editor. The navigation block is adding the list view button in the toolbar. Likely we'll remove this, and instead slot/fill from the editor, or find the appropriate hook to do so. One example is how we add a block settings menu item, but is using an unstable fill __unstableBlockSettingsMenuFirstItem
  • For the dropdown we should probably display Navigation Menu names as select option labels. In some cases the blocks won't have a name (when it has innerBlocks). When they do have names, data access is cumbersome, since we need to fetch post items with getEntityRecords. It's worth looking into localizing data a bit, or creating another store.

@gwwar
Copy link
Contributor Author

gwwar commented Feb 10, 2022

Heads up that I'm going to tidy up the summary

@gwwar gwwar added [Package] Block editor /packages/block-editor [Package] Block library /packages/block-library [Package] Edit Post /packages/edit-post [Package] Edit Site /packages/edit-site [Block] Navigation Affects the Navigation Block labels Feb 10, 2022
@gwwar gwwar marked this pull request as ready for review February 10, 2022 22:15
@gwwar gwwar requested a review from ellatrix as a code owner February 10, 2022 22:15
@gwwar gwwar requested a review from mtias February 10, 2022 22:23
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for trying this out! This is just a quick first pass from me, mainly testing; I haven't looked at the changeset in depth yet.

A few thoughts/questions:

  • Does it make sense to introduce this to the post editor, at least at this point? Because navs are being fetched from the editor content, and posts will most often not have them, it feels a bit unnecessary.
  • Reading through Introduce "Browser" and surface main navigation UI #36667 it looks like the aim is for this sidebar to be a representation of the site structure, with each nav link actually taking us to a view of its page in the editor. This will only work if the nav consists solely of a curated list of links. Page list block is opaque, so if the nav is just a page list, we won't benefit from this view at all. And other blocks that might be added, e.g. Search or Spacer or even Social Icons will muddy that representation quite a bit. I don't have any ideas here except that we should probably decide early on whether we want this list to function as a nav-editing device, or a site structure representation, because we'll need quite different solutions for each of these aims.
  • Wasn't this view meant to be on the left hand side of the editor? I noticed you also opened Navigation Sidebar: experiment with a persistent vertical display #38465, is that meant to complement this PR?
  • The list view does provide a much neater interface to move nav items around ⭐
  • We should probably look at a way to move items with the keyboard in list view (as a separate piece of work, of course 😅 )

/**
* WordPress dependencies
*/
import { useEntityProp } from '@wordpress/core-data';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can avoid having core-data as a dependency of the block editor package? Block editor should be usable independently from WP.

Could we perhaps add the title as an attribute of the nav block, and then use getBlock to access it instead?

@youknowriad
Copy link
Contributor

Thanks for the PR! This looks like a great start.

I think my main remark here is close to what @tellthemachines shared above: basically this PR is making the added sidebar part of the "block editor" like an "additional block inspector" blocks can use to trigger sidebars while it seems to me like the main goal of the original issue is to add something that is more "site wide" and not specific to the current post/page/template being edited. In other words, this should be a sidebar of the site editor (and probably not have anything to do with complementary editor areas), it's more global that the thing being current edited in the site editor.

I think reusing the list view UI is indeed the right approach but it doesn't have to be the exact same thing used by the navigation block's modal, that one is contextual to the current navigation block being edited in the canvas.

@draganescu
Copy link
Contributor

Nice work 👏🏻

I imagine that we don't want to necessarily convert these navigation menus automatically.

I think we do. We don't want to manage classic menus via a block structure as this has resulted in many setbacks in last year's explorations for adding a block editor for wp menus.

@gwwar
Copy link
Contributor Author

gwwar commented Feb 11, 2022

Thanks for the speedy feedback y'all, so to summarize what I'm hearing:

  • Keep this focused to the site editor for now. Perhaps pull this out from complementary areas
  • It sounds like it'd be helpful to also add some exploratory commits with the new site structure of pinned icons. (I had been planning to leave that as a follow up for PR scope reduction).
  • The new sidebar should be able to query the published list of menus. (Perhaps in the future we'd open this up to new menu creation/drafts as well?)
  • Think through how to populate a ListView or similar when the navigation block is not inserted in the edited template.
  • Data: I'd recommend this for a separate PR, but I'd agree that data lookup for navigation blocks are awkward, due to the multiple data sources and need to potentially fetch a core resource. We can push for more locality in our stores.
  • It's also an interesting thought to show the children of page list in a ListView component, though we'd likely need to lock items down.
  • Keyboard support: turn on mover controls. As a follow up find an alternate way for moving items in list view if desired.

Questions on behavior:

  • If I select a menu that is not inserted in one of the visible templates, should we render a preview of it in the main area?
  • If we change the information architecture of the site editor, will this also affect the post, navigation and widget editor? What does it look like when a plugin adds pinned icons?
  • The navigation block currently has an "open list view" button on the block toolbar. In the post editor, should we skip adding this button to the toolbar, or keep a modal view?

@gwwar
Copy link
Contributor Author

gwwar commented Feb 11, 2022

So to move to fetching all navigation items, from the package architecture page it sounds like it might make sense to add some wp_navigation resource querying in the @wordpresss/editor package. Let me know if there's a better spot.

@gwwar
Copy link
Contributor Author

gwwar commented Feb 11, 2022

Being able to load any menu gives us an interesting problem to solve. The ListView component currently assumes that any blocks it displays are available in the block-editor store.

Imagine that we're editing the index template, which has a navigation block that points at a wp_navigation ref of 123. If we select a menu with ref 456, ListView will not be able to render that menu since the innerBlocks of that menu are not in the template.

There a few ways of approaching this, but I think these two might be more promising.

Option A: Provided that there's at least one navigation block in the template, selecting a menu ref, also updates the block attribute. I pushed a happy path example to the branch. There's still corner cases to work through.

CleanShot.2022-02-11.at.13.53.18.mp4

Option B: Update ListView (or create a new component) to decouple dependence on specific data stores. If we pass a fully populated list of blocks, we should see a nice preview. @youknowriad had a preference for this one.
I can try drafting an alternate approach next week.

@gwwar gwwar removed [Package] Edit Site /packages/edit-site [Package] Edit Post /packages/edit-post labels Feb 11, 2022
@ciampo
Copy link
Contributor

ciampo commented Feb 21, 2022

Hey @jasmussen , thank you for the ping!

I just wanted to mention an experimental component that was recently added to the @wordpress/components package — Navigator. This component offers a way to declare "screens" and easily navigate between them (it is currently powering the Global Styles sidebar), without necessarily introducing any opinionated UI. It'd be interesting to see if it could be a good fit for this situation or, in case it isn't, understand the reason and see if we can iterate on it.

if we went this direction, what would be the work load attached to making the inspector work on a dark background?

Currently components from @wordpress/components don't support theming out of the box, so in order to implement such a scenario, the consumer of the components will have to pass ad-hoc styles to override colors etc.

But theming is definitely an area that we intend to explore in the future, and the work on the Navigation Menu Sidebar could be very useful in exploring how that could look like.

@jasmussen
Copy link
Contributor

Currently components from @wordpress/components don't support theming out of the box, so in order to implement such a scenario, the consumer of the components will have to pass ad-hoc styles to override colors etc.

But theming is definitely an area that we intend to explore in the future, and the work on the Navigation Menu Sidebar could be very useful in exploring how that could look like.

Thanks a bunch for chiming in. The missing theming is definitely a bit of a challenge for this PR, and even just the general concept of having inspector-like controls on a dark background, which is otherwise conceptually interesting. You can see the GIF here when diving into "All blocks", the controls start working less well there.

If we wanted a near-term solution to this, what's your best idea for accomplishing this? Color overrides in CSS could probably work, but we'd ideally want to do it in a way that didn't incur a ton of technical debt.

@gwwar
Copy link
Contributor Author

gwwar commented Feb 22, 2022

If we wanted a near-term solution to this, what's your best idea for accomplishing this? Color overrides in CSS could probably work

Zooming out a bit for theming requirements in components, I feel like there are a few different layers to this:

os settings Admin Theme Plugin theme
macos-big-sur-system-prefs-general-dark component theming CleanShot 2022-02-22 at 10 03 23@2x

OS Preferences for Dark/Light modes

If a user sets an OS level preference, the UI should ideally respond to this. If someone picks a dark mode, what does the current design look like for block editing areas?

Admin theme scheme

Beyond that users may also pick an admin theme. /wp-admin/profile.php. We do use this a bit in the block editor, but this is primarily currently limited to the accent color. Do existing themes have enough colors in the pallet to differentiate between the global/site settings and the document?

Plugin Theming

Finally, if @wordpress/components is used as a building block for building UI for plugins, it's highly likely that plugin developers will want to theme their UI with their product pallet. For example the yoast plugin may want to theme some components with rgb(164, 40, 106) background and #fec228 as a primary

@jameskoster
Copy link
Contributor

At the risk of sounding a bit boring, I think the Apple approach works very well. IE choose either light, dark, or auto mode, and then select an accent color to personalise.

Plugin theming feels like a whole other discussion. There's a question around the benefits of interfering with customer preferences in this way. Should branding be introduced another way?

@draganescu
Copy link
Contributor

I wan to just post some 👍🏻 s for the cool separation of global dark left and local white right, feels like something that is quickly learned without training.

In terms of navigation we should be able to create navigation in that area as well and I feel looking at the demos that we kinda need to introduce the concept of primary / secondary right there? Maybe, not sure.

it's probably more intuitive for most folks to navigate to a page on their site and edit from there, rather than expecting them to understand that the appearance of their homepage is governed by the "Index" template.

@jameskoster this being totally true, it's also a thing that for a page to be homepage the index template is assigned, so the thing becomes should we have some default site structure where there is a menu that by default contains a "Home" link?

@jameskoster
Copy link
Contributor

should we have some default site structure where there is a menu that by default contains a "Home" link?

That's a good question. I'd love to hear more thoughts on this, but I would be inclined to exclude the home link by default because the Site Title / Logo generally provide this functionality.

The question then becomes: how do we expose pages that are not part of the default menu in the site structure? That's probably something to explore separately, but perhaps there's an "unlinked" section below the menu? That could contain things like the home page, and perhaps even post categories, tags, and other archives. Folks would be able to simply drag items from the "unlinked" section to their menu if they want to add them.

@ciampo
Copy link
Contributor

ciampo commented Feb 23, 2022

@jasmussen:
The missing theming is definitely a bit of a challenge for this PR

We are definitely aware of that. Adding theming support (at least at a basic level) has already been discussed and we have an idea about how to approach it, although we haven't had the capacity to actually start experimenting on it yet (most of our time is spent helping folks with daily reviews, while slowly working on a few structural and fundamental package-wise improvements which will unblock future work).

We are still unsure about the specific requirements for the exposed theming APIs. In that regard, proceeding with style overrides on this project really is a helpful step because we can look at those overrides to understand what variables really need to be exposed, and we can keep that in mind so that the "theming" for this particular feature doesn't become too ambitious in the first place.

@jasmussen:
If we wanted a near-term solution to this, what's your best idea for accomplishing this? Color overrides in CSS could probably work, but we'd ideally want to do it in a way that didn't incur a ton of technical debt.

The best short-term solution would be to apply CSS overrides to these components where they are consumed (e.g. in the @wordpress/block-editor package), which we'd be removing once proper theming support is added natively to the components.

@gwwar:
Zooming out a bit for theming requirements in components, I feel like there are a few different layers to this

@jameskoster:
At the risk of sounding a bit boring, I think the Apple approach works very well. IE choose either light, dark, or auto mode, and then select an accent color to personalise.
Plugin theming feels like a whole other discussion. There's a question around the benefits of interfering with customer preferences in this way. Should branding be introduced another way?

The approach that was discussed on our end was to have the @wordpress/components package expose a few theming variables as CSS Custom properties.

It would be up to the consumer of @wordpress/components to establish the final value for each of these properties — for example, by default they could follow the "OS -> WP theme -> WP plugin" cascade mentioned above. But the Navigation Menu Sidebar could then override these settings so that every component rendered inside it would adopt a custom (dark) "theme".

@jasmussen
Copy link
Contributor

The question then becomes: how do we expose pages that are not part of the default menu in the site structure? That's probably something to explore separately, but perhaps there's an "unlinked" section below the menu? That could contain things like the home page, and perhaps even post categories, tags, and other archives. Folks would be able to simply drag items from the "unlinked" section to their menu if they want to add them.

That would be my instinct as well. There could be many pages, though, so we could need a button drawer for "more", so we at least avoid needing pagination.

We are still unsure about the specific requirements for the exposed theming APIs. In that regard, proceeding with style overrides on this project really is a helpful step because we can look at those overrides to understand what variables really need to be exposed, and we can keep that in mind so that the "theming" for this particular feature doesn't become too ambitious in the first place.

👍 👍

I would echo Jay's assessment of Kerry's proposal, though: keep it simple, so components retain their baked-in accessibility and DNA. So working on light and dark backgrounds would be step 1. System level dark mode is probably not near term for the project, so that's mostly something to keep in mind, but even with that, there will be cases in light mode where we need components that show up on a dark background, so it can't just be a blanket switch.

For any theming properties beyond light and dark, I would imagine a single accent color and possibly border radius. Both should probably be component-wide, i.e. don't mix multiple accent colors and radiuses across the UI. Fonts and font sizes should probably be inherited by the context, mainly. Does that sound right to you?

@ciampo
Copy link
Contributor

ciampo commented Feb 24, 2022

@jasmussen I agree with everything you said — we do want to keep things as simple as possible (especially during the first iterations), which includes exposing only the "theming variables" that strictly necessary. We'll definitely keep you, Jay and Kerry in mind for gather early feedback too!

@talldan
Copy link
Contributor

talldan commented Feb 25, 2022

I had a go at using inner blocks for Page List in #39087, which would solve the issue of it not showing its contents in this sidebar.

It doesn't actually seem too bad. The new block locking features bring us a lot closer to this being a possibility.

@getdave
Copy link
Contributor

getdave commented Mar 8, 2022

This is great work.

I'd like to look at breaking this PR down to the essentials required to merge a Navigation Menu (only) sidebar. I'll be testing this PR and disentangling what is reusable.

@getdave getdave mentioned this pull request Mar 8, 2022
13 tasks
@gwwar
Copy link
Contributor Author

gwwar commented Mar 8, 2022

@getdave I'd be happy to pull out those parts for you.

@gwwar gwwar force-pushed the try/nav-dedicated-sidebar branch from 5eb2efc to 57e65c0 Compare March 8, 2022 15:41
@gwwar
Copy link
Contributor Author

gwwar commented Mar 8, 2022

I rebased this branch to resolve conflicts.

I'd like to look at breaking this PR down to the essentials required to merge a Navigation Menu (only) sidebar.

@getdave I pulled this out for you in #39290 feel free to commit directly if you head ideas.

@gwwar gwwar added the [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible label Mar 8, 2022
@paaljoachim
Copy link
Contributor

paaljoachim commented Mar 9, 2022

Testing using this method: https://make.wordpress.org/design/2021/03/03/testing-a-gutenberg-pull-request-pr/

It is quite a contrast.

Before:
Screenshot 2022-03-09 at 21 46 06

After:
Screenshot 2022-03-09 at 21 49 36

My initial reactions.
I have a vertical black bar in my layout. As in a sense it looks like it is part of my layout. I wonder why the Navigation and Styles are placed like they are. As earlier Styles icon was placed on the top right area.
Clicking W I now see this:
Screenshot 2022-03-09 at 21 57 42
To me it becomes a bit confusing as the < Dashboard points toward the Nav and styles icon. The W menu area also becomes bigger.

What if the Nav and Styles were instead moved into the W menu area like so:
Moving Styles and Navigation into W menu

Here we are gradually recreating the left WordPress admin menu.


In addition to the adjustments I suggested above we could add the full Navigation into List view.

Currently we see this:
Screenshot 2022-03-09 at 22 07 16

What if we instead of the Page List actually saw when opening the Navigation in List view saw the full navigation and also moved the various links around. (Similar to how we can move things around in the current classic Navigation screen.)
List-view-Navigation-details

This way we are reusing an existing List view feature. Going into the details of a block. Being able to adjust the contents inside the block by using the List view.


Edit settings sidebar panel.
Adding a panel directly in the Navigation settings sidebar. Called for instance "Edit" to where one can manage the Navigation elements. Drag & Drop etc.
Edit-nav-in-sidebar

@getdave
Copy link
Contributor

getdave commented Mar 10, 2022

@paaljoachim Thank you for taking the time to review.

Unfortunately, this PR is actually pretty broad right now and it trying to tackle a number of things. I can't see it landing in 6.0.

Kerry ended up extracting a subset of this PR into #39290 which adds a more simple Navigation-only sidebar.

I think we should pursue the more limited scope outlined in #39290 else we're going to bite off more than we can chew.

It might be worth taking a look at that PR.

add the full Navigation into List view.

This works already. You are using the Page List block which currently doesn't expose inner blocks within the list view. Instead try adding some standard links to the Navigation block and then opening it in list view. You'll see you have full control of the Nav structure within the list view.

That said I don't currently believe list view can handle all the requirements necessary to fully manage a Navigation (think managing styles, block settings, editing text .etc). It's going to end up replicating the functionality which we already have within the editor canvas via blocks.

My preference would be for list view to remain "structure only" and for more dedicated approaches to handle the more complex parts of editing a Navigation.

@gwwar
Copy link
Contributor Author

gwwar commented Mar 10, 2022

Goals in #36667 are a bit broad.

To clarify a bit further the PR here is a technical prototype that quickly shows what it looks like UX might feel like with a dedicated vertical rail on the left for global panels, and how we might make it easier to manage navigation structure.

I think folks are leaning toward other UX options, so it's pretty unlikely that this PR will land.

I think it's still useful for folks to play with the branch, as it does quickly highlight some technical issues, especially if folks end up wanting to add additional functionality to ListView to make it more of a mini-editor. There's quite a number of side-effects that will need to be untangled from block list if that's so.

What if we instead of the Page List actually saw when opening the Navigation in List view saw the full navigation and also moved the various links around.

@paaljoachim, @talldan explores this in #39087

@paaljoachim
Copy link
Contributor

paaljoachim commented Mar 10, 2022

A few things.
I was confused with seeing the Page list (as it become a kind of grouping of nav elements) and had to modify, Save the Navigation to have it open up to show the individual links. I found it weird to see the Page list as part of the navigation. It felt out of place being in the Navigation elements list.

From my limited exploration it seems that List view is working fairly well with drag and drop of individual links. It should be kept simple and deeper methods are needed elsewhere.

I think we should pursue the more limited scope outlined in #39290 else we're going to bite off more than we can chew.

I went ahead and left feedback there. Bottom line that I find would be helpful would be to in some way connect the "Navigation Editor" with the Nav block sidebar panels. Because a deeper editing of the Navigation should be associated with the Navigation settings.

@scruffian
Copy link
Contributor

Created a rebase here: #39885

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 [Package] Block editor /packages/block-editor [Package] Block library /packages/block-library [Package] Edit Site /packages/edit-site [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.