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: Use gap instead of margin. #32367

Merged
merged 3 commits into from
Sep 2, 2021
Merged

Navigation: Use gap instead of margin. #32367

merged 3 commits into from
Sep 2, 2021

Conversation

jasmussen
Copy link
Contributor

Description

This PR refactors the navigation block to use gap instead of margins for spacing items out. The rules remain the same, if a navigation block has a background color, it has a smaller gap, than if it doesn't.

Submenu items should be unaffected, as those are not spaced out using margins at all, purely the padding of items themselves.

This comes with a few benefits:

  • If nav items wrap, the right-most item on row 1 doesn't have margin on its right.
  • The CSS is vastly simplified and reduced, making it easier for themes to override.
  • Paired with Global Styles: Consider supporting gap #32366, this could provide a neat interface directly in the navigation block for controlling item-spacing.

Testing instructions

Compare this branch with trunk, and the spacing should be the same. Before:

before

After:

after

Please test with and without backgrounds, vertical and horizontal, frontend and backend. Test a few themes. Some themes like TwentyTwentyOne provide style overrides for the navigation block that are obsolete, so won't necessarily be accurate, see also https://core.trac.wordpress.org/ticket/53220.

Screenshots

Screenshot 2021-06-01 at 12 25 32

Screenshot 2021-06-01 at 12 25 45

colors

Screenshot 2021-06-01 at 12 28 00

Screenshot 2021-06-01 at 12 28 12

Screenshot 2021-06-01 at 12 28 16

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

@jasmussen jasmussen added [Type] Code Quality Issues or PRs that relate to code quality [Block] Navigation Affects the Navigation Block CSS Styling Related to editor and front end styles, CSS-specific issues. labels Jun 1, 2021
@jasmussen jasmussen requested a review from ajitbohra as a code owner June 1, 2021 10:35
@jasmussen jasmussen self-assigned this Jun 1, 2021
@github-actions
Copy link

github-actions bot commented Jun 1, 2021

Size Change: -156 B (0%)

Total Size: 1.04 MB

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

compressed-size-action

Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

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

This tests well with just link menus. A minor concern would be the Can I Use stats it is available in most desktop and current mobile, but the previous Safari still show 11% usage.

Also, the margins are off for social links, you can see in the GIF they are up against the border while the links are not. This looks the same without this change, so not a regression, so if an issue can be addressed elsewhere.

gap-social

My biggest question for approving the change is lack of support in the older mobile Safari browser.

@jasmussen
Copy link
Contributor Author

Thanks for looking. Valid point about social links — and ultimately I think we should move those towards using gap as well: it's a much simpler way of ensuring harmonious spacing without an escalating set of width and nth-child rules for margins around wrapping elements.

In this particular case, the before/after should visually look identical, with and without gap for social links:

Screenshot 2021-07-01 at 10 59 34

The other part of your question is a good one: can we start to use gap more widely considering many are still using Safari 13? Utimately yes, I think so, because what happens if gap is not supported is a purely decorative issue, and not functional. So if not now, then at least soon. We landed this change for Buttons, making the same evaluation of complexity vs. legacy support. Navigation is perhaps a bit more prominent, with lack of support being potentially more noticable.

The other half of the equation is global styles support, as tracked in #32366. Right now you can actually change margins using global styles, but since you can't change the gap, arguably it would remove that feature (even if currently not used at all). Perhaps I should mark this one as blocked until #32366 is addressed, and then we can revaluate the browser support situation.

@jasmussen jasmussen added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Jul 1, 2021
@jasmussen jasmussen mentioned this pull request Aug 4, 2021
7 tasks
@jasmussen
Copy link
Contributor Author

Since #32659 has landed, this is now unblocked and can be picked up again, so I rebased it. One thing I noticed though, is that there's a bit of a markup discrepancy when combining the nav menu with social links. Here's the editor:

Screenshot 2021-08-09 at 09 09 48

Note how there's gap between all nav block items, and social links is treated as an item. This is the markup:

Screenshot 2021-08-09 at 09 10 42

Note how the social links is a descendant of wp-block-navigation__container.

Here's the frontend:

Screenshot 2021-08-09 at 09 10 58

Here, the gap doesn't affect the Social Links block at all, and the answer lies in the markup:

Screenshot 2021-08-09 at 09 11 05

Here, Social Links is a descendant directly of wp-block-navigation, not the container inside.

While this is something I can work with, I'm not sure this discrepancy is intentional, and if we could fix it it would be nice. @tellthemachines @draganescu do you know if this is intentional or something I could/should ticket?

@jasmussen jasmussen removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Aug 9, 2021
@tellthemachines
Copy link
Contributor

While this is something I can work with, I'm not sure this discrepancy is intentional, and if we could fix it it would be nice.

It is intentional, a result of the decision to decouple the front end from the editor markup in the Navigation block. The divergence is exactly because we need to wrap the Nav link items in a ul and we couldn't do so in the editor (or at least we can't without extensive changes to how inner blocks work, and we decided to do it this way instead because the markup differences were relatively minor).

For the styling to work across the two sets of markup, can we ignore the ul or div altogether and style the inner blocks as if they weren't wrapped?

If it's any help, we could remove the wp-block-navigation__container class from the div wrapper in the editor.

@jasmussen
Copy link
Contributor Author

It is intentional, a result of the decision to decouple the front end from the editor markup in the Navigation block. The divergence is exactly because we need to wrap the Nav link items in a ul and we couldn't do so in the editor (or at least we can't without extensive changes to how inner blocks work, and we decided to do it this way instead because the markup differences were relatively minor).

Ah, gotcha. I just meant that social links could still have been inside the wp-block-navigation__container, just needing a wrapping li of course.

For the styling to work across the two sets of markup, can we ignore the ul or div altogether and style the inner blocks as if they weren't wrapped?
If it's any help, we could remove the wp-block-navigation__container class from the div wrapper in the editor.

Yes, I think that's an option. Let me dive into the markup once more with this helpful context, and see what might work best 👍 👍

@jasmussen
Copy link
Contributor Author

Dove in for a bit, and formed two thoughts. First off, it does seem like the wp-block-navigation__container is not supremely useful as a wrapping div. It doesn't seem to serve an important purpose. However it also doesn't seem like it would solve the discrepancy; the idea being that the social links block is a block just like a menu item, so ideally they're both children of the same container. Can we do that? I.e. make the social links block a child of the wp-block-navigation__container UL, like so:

Screenshot 2021-08-11 at 10 33 46

In quick debugging it seems like it would work and be valid:

diff

It's entirely possible I'm missing a markup nuance though, and I can work with the existing markup if need be, let me know!

@tellthemachines
Copy link
Contributor

First off, it does seem like the wp-block-navigation__container is not supremely useful as a wrapping div. It doesn't seem to serve an important purpose.

That div is only there on the editor side because we need an element that's not the outer nav wrapper to attach the inner blocks logic to; it doesn't serve any purpose visually. We need a div in that place, but we don't need it to have a classname, so we can remove that at least.

Can we do that? I.e. make the social links block a child of the wp-block-navigation__container UL

Sure, we can do that. My only reservation is that, though it's semantically correct, it might not make sense in terms of the content.

The only other nested uls we have are submenus, and those have a top level item that specifies their place as sub-lists of a main list, whereas the social icons wouldn't have that. As content, they are unlikely to be related to the other menu items, so it's hard to tell why they should be a sub-list. The fact that social icons is its own block already points to that, otherwise we might have a social icon menu item type allowing us to intersperse individual social links with other menu items.

And due to the way we wrap menu items in uls on the server side, we could easily end up with something nonsensical like this:

<nav class="wp-block-navigation">
  <ul class="wp-block-navigation__container">
    <li class="wp-block-navigation-link">...</li>
  </ul>
  <form role="search">...</form>
  <ul class="wp-block-navigation__container">
    <li>
      <ul class="wp-block-social-links">
        <li class="wp-block-social-link">...</li>
      </ul>
    </li>
  </ul>
</nav>

Having said that, if it does make it a lot easier to style the nav consistently, we should still consider doing it.

@jasmussen
Copy link
Contributor Author

All excellent points! I think I'll pause this one until the new unified CSS classes have landed. The followup refactor will likely make it much more clear what the best path forward is. Thanks for your input 👌

@jasmussen jasmussen added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Aug 19, 2021
@jasmussen
Copy link
Contributor Author

Rebased this one and fixed an issue also outlined in #34351.

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.

Rebased this PR so we can test with the recently-merged #33316. It's working well! The only part that isn't working is the Pages List styles; I left comments on that below.

Also, I now see why you were apprehensive about the Social Icons markup:
Screen Shot 2021-09-01 at 1 58 57 pm

The only way I could find of getting all these items to line up was setting everything to display: inline, but that won't allow justification, so not a viable solution.

I like the idea of allowing individual social icons in the Nav, instead of being grouped in a Social Icons block. Can you think of any reasons not to try that? If not, I'm happy to give it a go in another PR 😄

&:last-child {
margin-right: 0;
}
.wp-block-navigation__container .wp-block-page-list,
Copy link
Contributor

Choose a reason for hiding this comment

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

On the front end, wp-block-page-list isn't inside wp-block-navigation__container, so this won't apply. I think we could go with .wp-block-navigation .wp-block-page-list instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent, that worked:

Screenshot 2021-09-01 at 10 20 27

Screenshot 2021-09-01 at 10 20 36

&:last-child {
margin: 0;
}
.wp-block-navigation__container .wp-block-page-list,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@jasmussen
Copy link
Contributor Author

Thank you for the rebase and the review. Let me address your comments and see what I can do about the page list.

@jasmussen jasmussen removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Sep 1, 2021
@jasmussen
Copy link
Contributor Author

Forgot to answer this one:

I like the idea of allowing individual social icons in the Nav, instead of being grouped in a Social Icons block. Can you think of any reasons not to try that? If not, I'm happy to give it a go in another PR 😄

The only downside is that the block inserter becomes very unwieldy having items like Page Link intermixed with every social link under the sun. #34041 should mitigate that, especially if paired with some way to curate the quick inserter. I would definitely agree that it's a better experience to not have to have an extra wrapper just for social links.

@jasmussen
Copy link
Contributor Author

I guess this one is mostly ready! I had some comments in #33812 (comment) on how to better integrate with global styles, and perhaps make the navigation block a flex container as defined in block.json rather than in CSS as is done here. But that's probably best left as a followup.

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.

Page list is looking much better now 🎉

There's a slight discrepancy in spacing between regular items and page list items due to the padding applied to wp-block-navigation-link__content, especially noticeable in the vertical nav:
Screen Shot 2021-09-02 at 10 21 52 am
But that's unrelated to these changes so let's fix it separately.

The only downside is that the block inserter becomes very unwieldy having items like Page Link intermixed with every social link under the sun.

Good point 😄 I guess it comes down to whether it's a better experience to have all those links directly available, or to have to add an intermediate block in order to access them.

If we do want to keep using the Social Links block, another thing we could try is nesting the whole block inside the wp-block-navigation__container and then adding role="presentation" to the extra wrapping li and ul so that semantically the social links are siblings of the main list items. I haven't actually tried this yet, so we'd need to test it well to make sure it works. In terms of the layout though, having that extra wrapper in place means that individual social links won't be able to wrap like the other nav items - much like what currently happens with Page List.

@tellthemachines
Copy link
Contributor

There's a slight discrepancy in spacing between regular items and page list items due to the padding applied to wp-block-navigation-link__content, especially noticeable in the vertical nav:

Actually forget it, those are theme styles, aren't they 🤦

@jasmussen
Copy link
Contributor Author

Actually forget it, those are theme styles, aren't they 🤦

If you were using TT1 to test, there are some known issues with menu item paddings and margins there. I did some testing but couldn't reproduce, but if we find a way to reproduce I'll be happy to follow up and fix!

Good point 😄 I guess it comes down to whether it's a better experience to have all those links directly available, or to have to add an intermediate block in order to access them.

Honestly I think your point is well made, and that the issue is worth solving. Some of the efforts outlined in #34041 will need to handle these cases anyway, and a curated quick inserter I honestly believe would benefit all blocks with nesting.

Thank you for the reviews. I think I'll land this one and follow up if anything comes up.

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 CSS Styling Related to editor and front end styles, CSS-specific issues. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants