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

Navigation: Add an option to allow navigation to switch to overlay mode when wrapping #57587

Closed
wants to merge 28 commits into from

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented Jan 5, 2024

What?

Addresses #45040

https://codepen.io/Marga-Cabrera/pen/WNPjYYw

This PR adds a new option for the nav block overlay called "auto". When this option is enabled a script will detect if the nav block can fit within its own container. If it detects that it's wrapping, it will toggle the overlay burger automatically, instead of using the fixed breakpoint.

We do two checks here:

  • First we check if the elements within our known block are wrapping. This is done simply by comparing the width of the elements with the width of the container, but this is not the only case, in fact this is the least common occurrence
  • Most of the time, the nav block will be inside a group or column block and appear alongside other blocks such as the site logo or site title/tagline which could cause the wrapping to occur. We can't predict those, and I'm only making one assumption here that I think is valid (but could change in the future): the wrapping will be cause from a parent that is using flex-wrap. Based on that assumption I look for said parent recursively and apply the same principle to verify if the children inside it are wrapping or not. In my personal opinion, if we are not covering this case, there is very little use in continuing this path, since 90% of the time, this is what's causing the nav block to wrap ahead of time and not the amount of items inside it.

To do:

  • The "Allow to wrap to multiple lines" control doesn't make sense in this context, we should hide it (can be a follow-up).
  • Check that the menu is horizontal before showing the breakpoint .
  • Test on multiple header styles.
  • Add tests.

Why?

After the discussion over at #54388 and some first explorations, it becomes really clear that we either allow our users to set a breakpoint manually or we need to use JavaScript to detect when. Given the complexity of the nav block and how many combinations of layouts it can be wrapped in, there is no way CSS can only give a solution that will work for most of the use cases.

How?

Testing Instructions

  • Create a navigation menu that has a site logo and a submenu in it.
  • Insert a navigation block and set it to Auto
  • Resize the viewport to check that the overlay appears at the moment when the menu is about to wrap, either because the nav items no longer fit the container or because the navigation block wraps because it's too big inside its own wrapper
  • Test this with different header layouts, the ones I've used are in this gist

Testing Instructions for Keyboard

Screenshots or screencast

This adds a new option to the toggle:
Screenshot 2024-01-11 at 11 11 51

I don't think this is clear enough on its own. Maybe it should be called wrap? Maybe we should use a dropdown instead of the toggle group? cc @WordPress/gutenberg-design

@scruffian scruffian added [Type] Enhancement A suggestion for improvement. [Block] Navigation Affects the Navigation Block labels Jan 5, 2024
@scruffian scruffian self-assigned this Jan 5, 2024
Copy link

github-actions bot commented Jan 5, 2024

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/experimental/editor-settings.php
❔ lib/experiments-page.php

Copy link

github-actions bot commented Jan 5, 2024

Size Change: +1.92 kB (0%)

Total Size: 1.7 MB

Filename Size Change
build/block-editor/content-rtl.css 4.35 kB -30 B (-1%)
build/block-editor/content.css 4.35 kB -31 B (-1%)
build/block-editor/index.min.js 250 kB +406 B (0%)
build/block-editor/style-rtl.css 15.5 kB -16 B (0%)
build/block-editor/style.css 15.5 kB -17 B (0%)
build/block-library/blocks/form/view.min.js 471 B +19 B (+4%)
build/block-library/blocks/navigation/view.min.js 1.52 kB +424 B (+39%) 🚨
build/block-library/blocks/social-links/style-rtl.css 1.49 kB -4 B (0%)
build/block-library/blocks/social-links/style.css 1.48 kB -4 B (0%)
build/block-library/index.min.js 215 kB +555 B (0%)
build/block-library/style-rtl.css 14.7 kB -5 B (0%)
build/block-library/style.css 14.7 kB -5 B (0%)
build/blocks/index.min.js 51.6 kB +89 B (0%)
build/components/index.min.js 235 kB +122 B (0%)
build/compose/index.min.js 12.6 kB +8 B (0%)
build/core-data/index.min.js 72.7 kB +49 B (0%)
build/date/index.min.js 17.9 kB +6 B (0%)
build/edit-site/index.min.js 195 kB +368 B (0%)
build/edit-site/style-rtl.css 15.3 kB +21 B (0%)
build/edit-site/style.css 15.3 kB +16 B (0%)
build/edit-widgets/index.min.js 17.3 kB +42 B (0%)
build/edit-widgets/style-rtl.css 4.23 kB -249 B (-6%)
build/edit-widgets/style.css 4.23 kB -251 B (-6%)
build/editor/index.min.js 61.6 kB +3 B (0%)
build/editor/style-rtl.css 5.43 kB +12 B (0%)
build/editor/style.css 5.43 kB +13 B (0%)
build/interactivity/navigation.min.js 1.65 kB +417 B (+34%) 🚨
build/patterns/index.min.js 5.44 kB -17 B (0%)
build/rich-text/index.min.js 10.4 kB -19 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 955 B
build/annotations/index.min.js 2.69 kB
build/api-fetch/index.min.js 2.32 kB
build/autop/index.min.js 2.1 kB
build/blob/index.min.js 578 B
build/block-directory/index.min.js 7.22 kB
build/block-directory/style-rtl.css 1.02 kB
build/block-directory/style.css 1.02 kB
build/block-editor/default-editor-styles-rtl.css 381 B
build/block-editor/default-editor-styles.css 381 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 90 B
build/block-library/blocks/archives/style.css 90 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 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 126 B
build/block-library/blocks/audio/theme.css 126 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 104 B
build/block-library/blocks/avatar/style.css 104 B
build/block-library/blocks/block/editor-rtl.css 305 B
build/block-library/blocks/block/editor.css 305 B
build/block-library/blocks/button/editor-rtl.css 415 B
build/block-library/blocks/button/editor.css 414 B
build/block-library/blocks/button/style-rtl.css 627 B
build/block-library/blocks/button/style.css 626 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 113 B
build/block-library/blocks/categories/editor.css 112 B
build/block-library/blocks/categories/style-rtl.css 124 B
build/block-library/blocks/categories/style.css 124 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 121 B
build/block-library/blocks/code/style.css 121 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 421 B
build/block-library/blocks/columns/style.css 421 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-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 199 B
build/block-library/blocks/comment-template/style.css 198 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-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 840 B
build/block-library/blocks/comments/editor.css 839 B
build/block-library/blocks/comments/style-rtl.css 637 B
build/block-library/blocks/comments/style.css 636 B
build/block-library/blocks/cover/editor-rtl.css 647 B
build/block-library/blocks/cover/editor.css 650 B
build/block-library/blocks/cover/style-rtl.css 1.69 kB
build/block-library/blocks/cover/style.css 1.68 kB
build/block-library/blocks/details/editor-rtl.css 65 B
build/block-library/blocks/details/editor.css 65 B
build/block-library/blocks/details/style-rtl.css 98 B
build/block-library/blocks/details/style.css 98 B
build/block-library/blocks/embed/editor-rtl.css 322 B
build/block-library/blocks/embed/editor.css 322 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 126 B
build/block-library/blocks/embed/theme.css 126 B
build/block-library/blocks/file/editor-rtl.css 316 B
build/block-library/blocks/file/editor.css 316 B
build/block-library/blocks/file/style-rtl.css 280 B
build/block-library/blocks/file/style.css 281 B
build/block-library/blocks/file/view.min.js 316 B
build/block-library/blocks/footnotes/style-rtl.css 201 B
build/block-library/blocks/footnotes/style.css 199 B
build/block-library/blocks/form-input/editor-rtl.css 227 B
build/block-library/blocks/form-input/editor.css 227 B
build/block-library/blocks/form-input/style-rtl.css 343 B
build/block-library/blocks/form-input/style.css 343 B
build/block-library/blocks/form-submission-notification/editor-rtl.css 340 B
build/block-library/blocks/form-submission-notification/editor.css 340 B
build/block-library/blocks/form-submit-button/style-rtl.css 69 B
build/block-library/blocks/form-submit-button/style.css 69 B
build/block-library/blocks/freeform/editor-rtl.css 2.61 kB
build/block-library/blocks/freeform/editor.css 2.61 kB
build/block-library/blocks/gallery/editor-rtl.css 947 B
build/block-library/blocks/gallery/editor.css 952 B
build/block-library/blocks/gallery/style-rtl.css 1.72 kB
build/block-library/blocks/gallery/style.css 1.72 kB
build/block-library/blocks/gallery/theme-rtl.css 108 B
build/block-library/blocks/gallery/theme.css 108 B
build/block-library/blocks/group/editor-rtl.css 654 B
build/block-library/blocks/group/editor.css 654 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 189 B
build/block-library/blocks/heading/style.css 189 B
build/block-library/blocks/html/editor-rtl.css 336 B
build/block-library/blocks/html/editor.css 337 B
build/block-library/blocks/image/editor-rtl.css 863 B
build/block-library/blocks/image/editor.css 862 B
build/block-library/blocks/image/style-rtl.css 1.6 kB
build/block-library/blocks/image/style.css 1.59 kB
build/block-library/blocks/image/theme-rtl.css 126 B
build/block-library/blocks/image/theme.css 126 B
build/block-library/blocks/image/view.min.js 2.01 kB
build/block-library/blocks/latest-comments/style-rtl.css 357 B
build/block-library/blocks/latest-comments/style.css 357 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 478 B
build/block-library/blocks/latest-posts/style.css 478 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 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 505 B
build/block-library/blocks/media-text/style.css 503 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 668 B
build/block-library/blocks/navigation-link/editor.css 669 B
build/block-library/blocks/navigation-link/style-rtl.css 103 B
build/block-library/blocks/navigation-link/style.css 103 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 296 B
build/block-library/blocks/navigation-submenu/editor.css 295 B
build/block-library/blocks/navigation/editor-rtl.css 2.25 kB
build/block-library/blocks/navigation/editor.css 2.26 kB
build/block-library/blocks/navigation/style-rtl.css 2.24 kB
build/block-library/blocks/navigation/style.css 2.23 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 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 235 B
build/block-library/blocks/paragraph/editor.css 235 B
build/block-library/blocks/paragraph/style-rtl.css 335 B
build/block-library/blocks/paragraph/style.css 335 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/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 508 B
build/block-library/blocks/post-comments-form/style.css 508 B
build/block-library/blocks/post-content/editor-rtl.css 74 B
build/block-library/blocks/post-content/editor.css 74 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 71 B
build/block-library/blocks/post-excerpt/editor.css 71 B
build/block-library/blocks/post-excerpt/style-rtl.css 141 B
build/block-library/blocks/post-excerpt/style.css 141 B
build/block-library/blocks/post-featured-image/editor-rtl.css 666 B
build/block-library/blocks/post-featured-image/editor.css 662 B
build/block-library/blocks/post-featured-image/style-rtl.css 342 B
build/block-library/blocks/post-featured-image/style.css 342 B
build/block-library/blocks/post-navigation-link/style-rtl.css 215 B
build/block-library/blocks/post-navigation-link/style.css 214 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 409 B
build/block-library/blocks/post-template/style.css 408 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-time-to-read/style-rtl.css 69 B
build/block-library/blocks/post-time-to-read/style.css 69 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 125 B
build/block-library/blocks/preformatted/style.css 125 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 354 B
build/block-library/blocks/pullquote/style.css 354 B
build/block-library/blocks/pullquote/theme-rtl.css 168 B
build/block-library/blocks/pullquote/theme.css 168 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 288 B
build/block-library/blocks/query-pagination/style.css 284 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 486 B
build/block-library/blocks/query/editor.css 486 B
build/block-library/blocks/query/style-rtl.css 312 B
build/block-library/blocks/query/style.css 308 B
build/block-library/blocks/query/view.min.js 1.1 kB
build/block-library/blocks/quote/style-rtl.css 237 B
build/block-library/blocks/quote/style.css 237 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 140 B
build/block-library/blocks/read-more/style.css 140 B
build/block-library/blocks/rss/editor-rtl.css 149 B
build/block-library/blocks/rss/editor.css 149 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 184 B
build/block-library/blocks/search/editor.css 184 B
build/block-library/blocks/search/style-rtl.css 614 B
build/block-library/blocks/search/style.css 614 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/search/view.min.js 471 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 229 B
build/block-library/blocks/separator/style.css 229 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 323 B
build/block-library/blocks/shortcode/editor.css 323 B
build/block-library/blocks/site-logo/editor-rtl.css 754 B
build/block-library/blocks/site-logo/editor.css 754 B
build/block-library/blocks/site-logo/style-rtl.css 204 B
build/block-library/blocks/site-logo/style.css 204 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 116 B
build/block-library/blocks/site-title/editor.css 116 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 682 B
build/block-library/blocks/social-links/editor.css 681 B
build/block-library/blocks/spacer/editor-rtl.css 348 B
build/block-library/blocks/spacer/editor.css 348 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 395 B
build/block-library/blocks/table/editor.css 395 B
build/block-library/blocks/table/style-rtl.css 639 B
build/block-library/blocks/table/style.css 639 B
build/block-library/blocks/table/theme-rtl.css 146 B
build/block-library/blocks/table/theme.css 146 B
build/block-library/blocks/tag-cloud/style-rtl.css 251 B
build/block-library/blocks/tag-cloud/style.css 253 B
build/block-library/blocks/template-part/editor-rtl.css 403 B
build/block-library/blocks/template-part/editor.css 403 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/style-rtl.css 111 B
build/block-library/blocks/term-description/style.css 111 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 99 B
build/block-library/blocks/verse/style.css 99 B
build/block-library/blocks/video/editor-rtl.css 552 B
build/block-library/blocks/video/editor.css 555 B
build/block-library/blocks/video/style-rtl.css 185 B
build/block-library/blocks/video/style.css 185 B
build/block-library/blocks/video/theme-rtl.css 126 B
build/block-library/blocks/video/theme.css 126 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.1 kB
build/block-library/common.css 1.1 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 12.3 kB
build/block-library/editor.css 12.3 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/reset-rtl.css 472 B
build/block-library/reset.css 472 B
build/block-library/theme-rtl.css 688 B
build/block-library/theme.css 693 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.87 kB
build/commands/index.min.js 15.5 kB
build/commands/style-rtl.css 921 B
build/commands/style.css 918 B
build/components/style-rtl.css 12 kB
build/components/style.css 12 kB
build/core-commands/index.min.js 2.71 kB
build/customize-widgets/index.min.js 12.1 kB
build/customize-widgets/style-rtl.css 1.34 kB
build/customize-widgets/style.css 1.33 kB
build/data-controls/index.min.js 640 B
build/data/index.min.js 8.92 kB
build/deprecated/index.min.js 451 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.65 kB
build/edit-post/classic-rtl.css 544 B
build/edit-post/classic.css 545 B
build/edit-post/index.min.js 25 kB
build/edit-post/style-rtl.css 5.67 kB
build/edit-post/style.css 5.66 kB
build/element/index.min.js 4.83 kB
build/escape-html/index.min.js 537 B
build/format-library/index.min.js 7.85 kB
build/format-library/style-rtl.css 478 B
build/format-library/style.css 477 B
build/hooks/index.min.js 1.55 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.58 kB
build/interactivity/file.min.js 440 B
build/interactivity/image.min.js 2.15 kB
build/interactivity/index.min.js 12.8 kB
build/interactivity/query.min.js 884 B
build/interactivity/router.min.js 971 B
build/interactivity/search.min.js 610 B
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.74 kB
build/keycodes/index.min.js 1.46 kB
build/list-reusable-blocks/index.min.js 2.11 kB
build/list-reusable-blocks/style-rtl.css 836 B
build/list-reusable-blocks/style.css 836 B
build/media-utils/index.min.js 2.9 kB
build/modules/importmap-polyfill.min.js 12.2 kB
build/notices/index.min.js 948 B
build/nux/index.min.js 2 kB
build/nux/style-rtl.css 735 B
build/nux/style.css 732 B
build/patterns/style-rtl.css 540 B
build/patterns/style.css 539 B
build/plugins/index.min.js 1.8 kB
build/preferences-persistence/index.min.js 2.07 kB
build/preferences/index.min.js 2.81 kB
build/preferences/style-rtl.css 698 B
build/preferences/style.css 700 B
build/primitives/index.min.js 975 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 1 kB
build/react-i18n/index.min.js 623 B
build/react-refresh-entry/index.min.js 9.47 kB
build/react-refresh-runtime/index.min.js 6.78 kB
build/redux-routine/index.min.js 2.7 kB
build/reusable-blocks/index.min.js 2.72 kB
build/reusable-blocks/style-rtl.css 243 B
build/reusable-blocks/style.css 243 B
build/router/index.min.js 1.79 kB
build/server-side-render/index.min.js 1.95 kB
build/shortcode/index.min.js 1.39 kB
build/style-engine/index.min.js 2.07 kB
build/token-list/index.min.js 582 B
build/url/index.min.js 3.72 kB
build/vendors/inert-polyfill.min.js 2.48 kB
build/vendors/react-dom.min.js 41.8 kB
build/vendors/react.min.js 4.02 kB
build/viewport/index.min.js 957 B
build/warning/index.min.js 249 B
build/widgets/index.min.js 7.21 kB
build/widgets/style-rtl.css 1.15 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.02 kB

compressed-size-action

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.

Performance wise the flickering is probably resulting from layout thrashing.

Below is a good resource for working out which properties read/write will cause this.

https://gist.github.com/paulirish/5d52fb081b3570c81e3a

packages/block-library/src/navigation/view.js Outdated Show resolved Hide resolved
import { NAVIGATION_MOBILE_COLLAPSE } from './constants';
import navigationIsWrapping from './is-wrapping';

function useIsCollapsed( overlayMenu, navRef ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something about this hook is buggy. Its possible to see a wrapped navigation block in the editor.

@scruffian scruffian marked this pull request as ready for review January 5, 2024 17:37
@MaggieCabrera

This comment was marked as outdated.

Comment on lines 593 to 596
<ToggleGroupControlOption
value="auto"
label={ __( 'Auto' ) }
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check if the nav block is set to show horizontal/vertical before showing this option? It doesn't make much sense if it's vertical

@scruffian scruffian added the Needs Design Needs design efforts. label Jan 11, 2024
@MaggieCabrera
Copy link
Contributor

There is a case that has not been considered: when the navigation is in a flex container but it's the first element inside it. If it's big enough, it will make whatever siblings are in the wrapper move underneath it and it will only trigger the overlay when there is no more space for the nav items:

<!-- wp:group {"style":{"spacing":{"padding":{"bottom":"var:preset|spacing|30"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group" style="padding-bottom:var(--wp--preset--spacing--30)">
	<!-- wp:group {"align":"wide","layout":{"type":"flex","flexWrap":"wrap","justifyContent":"space-between"}} -->
	<div class="wp-block-group alignwide"><!-- wp:group {"layout":{"type":"flex","justifyContent":"left","flexWrap":"wrap"}} -->
	<div class="wp-block-group"><!-- wp:site-title {"level":0} /-->
	
	<!-- wp:navigation {"overlayMenu":"auto","icon":"menu","layout":{"type":"flex","setCascadingProperties":true,"justifyContent":"left"}} /--></div>
	<!-- /wp:group -->
	
	<!-- wp:group {"style":{"layout":{"selfStretch":"fill","flexSize":null}},"layout":{"type":"default"}} -->
	<div class="wp-block-group"><!-- wp:social-links {"iconColor":"contrast","iconColorValue":"#e6e6dc","className":"is-style-logos-only","layout":{"type":"flex","justifyContent":"right","flexWrap":"nowrap"}} -->
	<ul class="wp-block-social-links has-icon-color is-style-logos-only"><!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /-->
	
	<!-- wp:social-link {"url":"github.com/WordPress","service":"github"} /-->
	
	<!-- wp:social-link {"url":"https://www.linkedin.com/company/wordpress","service":"linkedin"} /--></ul>
	<!-- /wp:social-links --></div>
	<!-- /wp:group --></div>
	<!-- /wp:group --></div>
	<!-- /wp:group -->

I think that makes sense and don't think we should consider (it would be too much complexity) but I don't know if RTL languages will use this pattern often or not. In any case, I think it shouldn't be considered a bug of this implementation.

@jameskoster
Copy link
Contributor

I don't think this is clear enough on its own. Maybe it should be called wrap? Maybe we should use a dropdown instead of the toggle group?

"Auto" seems reasonable to me, "Overflow" might be another option. We may need to switch to a dropdown regardless accounting for i18n. This would present an option to include a description for each value which might help too?

@getdave getdave mentioned this pull request Jan 11, 2024
38 tasks
@MaggieCabrera
Copy link
Contributor

I don't think this is clear enough on its own. Maybe it should be called wrap? Maybe we should use a dropdown instead of the toggle group?

"Auto" seems reasonable to me, "Overflow" might be another option. We may need to switch to a dropdown regardless accounting for i18n. This would present an option to include a description for each value which might help too?

I think a description would be really helpful

@scruffian
Copy link
Contributor Author

I updated the design to use a SelectComponent

Screenshot 2024-01-11 at 16 41 16 Screenshot 2024-01-11 at 16 41 11

}
window.addEventListener( 'resize', debounce( updateIsCollapsed, 100 ) );
return () => {
window.removeEventListener( 'resize', updateIsCollapsed );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that the function attached to the resize event is debounced, I'm not sure this will work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. You'll have to define a variable that refers to the debounced function and then provide that variable to the removeEventListener. Debounce returns a new function reference.

@liviopv
Copy link

liviopv commented Jan 11, 2024

This would be a nice addition to the block if it can detect the context accurately, which I understand is the main concern.

From my experience dealing with low-code end-users, block structures can get wild (unintentionally, in most cases) with Groups within Groups within Columns (sometimes a single column) and so forth. If this option can't parse through that and understand which width constraints are "artificial" and which are real, I feel like this could be a source of confusion.

Regarding the ways the option is shown, I much prefer the select option because it's more descriptive (also "auto" would be hard to distinguish from "mobile"), but I think "Navigation Overlay" doesn't feel describe what that option does. I wonder if we could go with a more common terminology, like "Navigation Responsiveness", "Responsive Navigation", "Mobile Navigation", "Mobile Behavior" or similar.

@MaggieCabrera
Copy link
Contributor

Regarding the ways the option is shown, I much prefer the select option because it's more descriptive (also "auto" would be hard to distinguish from "mobile"), but I think "Navigation Overlay" doesn't feel describe what that option does. I wonder if we could go with a more common terminology, like "Navigation Responsiveness", "Responsive Navigation", "Mobile Navigation", "Mobile Behavior" or similar.

The word mobile doesn't really work here, because only one of the options relies on the width of the viewport to trigger the burger menu (Mobile). Never will never trigger it, Auto depends on content, not viewport size and Always will show the overlay on "desktop" too. Wording is really hard, but I think responsive/mobile descriptors here are misleading.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

This is a great approach to the problem. It does indeed not solve setting a breakpoint, but it does alleviate the actual need for that in many situations, in a clever way 👏🏻 I think that:

  • it should be, like Joen suggested, the default behavior so that in the future we may improve on the implementation (so we should add no new UI for this)
  • it can be the default behavior if we make sure we don't have too many obvious edge cases
  • maybe, for the really edge edge cases people can select the other modes for collapsing behavior

What I don't like, personally, in this PR is that we also refactored other code so when we introduce the experiment we may leak some other changes. It would be great if we could not do the code shuffling even if that leads to duplication for the experiment.

Copy link

github-actions bot commented Feb 2, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: scruffian <scruffian@git.wordpress.org>
Co-authored-by: MaggieCabrera <onemaggie@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: draganescu <andraganescu@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: liviopv <liviopv@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@getdave
Copy link
Contributor

getdave commented Feb 2, 2024

it should be, like Joen suggested, the default behavior so that in the future we may improve on the implementation (so we should add no new UI for this)

...but only for newly inserted Nav blocks. If you have already got a Nav block and you left the default settings, I don't think we should be amending those defaults. Otherwise we're going to see a number of sites break if/when this becomes a non-expertiment.

@MaggieCabrera
Copy link
Contributor

@scruffian I think this is not the right way to go, especially thinking that the grid block is getting more work on and that will definitely make this solution way more complicated than it is. I don't want to close your PR even though we both worked on it

@scruffian scruffian closed this Feb 22, 2024
@scruffian scruffian deleted the add/auto-wrapping branch February 22, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block Needs Design Needs design efforts. [Type] Enhancement A suggestion for improvement.
Projects
Development

Successfully merging this pull request may close these issues.

7 participants