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

Change Navigation block markup on front end only #30551

Merged
merged 6 commits into from
Jul 7, 2021
Merged

Conversation

tellthemachines
Copy link
Contributor

@tellthemachines tellthemachines commented Apr 7, 2021

Description

Follow-up exploration into minimising impact of nav semantics for end users. This removes the <ul> from the Navigation block markup and adds it conditionally on innerBlock type, only on the server-side render, so it only applies on the front end.

Disadvantages: markup will be different between editor and front end; editor markup will be unsemantic with <li> elements nested directly inside the <nav> (we can fiddle with the Navigation link block and change its editor output too if we want to fix this). UPDATE: this has now been fixed, to render all link children of the Nav as divs in the editor.

Advantages: users don't have to bother about adding intermediary list blocks.

NOTE: Spacer blocks are (unsemantically) added inside the ul elements for now, as I was given to understand that Spacers would be required to work inside submenus too. If this is not the case, I can remove them from the lists. Otherwise we'll need to think of another way to address the Spacer issue.

The Spacer block has been temporarily removed from submenus and will be addressed separately; #33018 and #30590 explain what is needed.

With the changes in this PR, the Navigation block should contain fully semantic markup, no matter what its contents are.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@github-actions
Copy link

github-actions bot commented Apr 7, 2021

Size Change: +6.71 kB (+1%)

Total Size: 1.05 MB

Filename Size Change
build/a11y/index.min.js 1.12 kB +1 B (0%)
build/block-directory/index.min.js 6.62 kB +5 B (0%)
build/block-directory/style-rtl.css 1.02 kB +31 B (+3%)
build/block-directory/style.css 1.02 kB +31 B (+3%)
build/block-editor/index.min.js 120 kB +310 B (0%)
build/block-editor/style-rtl.css 13.8 kB +368 B (+3%)
build/block-editor/style.css 13.8 kB +365 B (+3%)
build/block-library/blocks/latest-comments/style-rtl.css 286 B +5 B (+2%)
build/block-library/blocks/latest-comments/style.css 286 B +4 B (+1%)
build/block-library/blocks/latest-posts/style-rtl.css 534 B +5 B (+1%)
build/block-library/blocks/latest-posts/style.css 532 B +3 B (+1%)
build/block-library/blocks/media-text/editor-rtl.css 263 B +87 B (+49%) 🚨
build/block-library/blocks/media-text/editor.css 264 B +88 B (+50%) 🆘
build/block-library/blocks/navigation/style-rtl.css 1.65 kB +16 B (+1%)
build/block-library/blocks/navigation/style.css 1.65 kB +18 B (+1%)
build/block-library/blocks/site-logo/editor-rtl.css 646 B +206 B (+47%) 🚨
build/block-library/blocks/site-logo/editor.css 647 B +206 B (+47%) 🚨
build/block-library/common-rtl.css 1.29 kB +28 B (+2%)
build/block-library/common.css 1.29 kB +28 B (+2%)
build/block-library/editor-rtl.css 9.75 kB +46 B (0%)
build/block-library/editor.css 9.75 kB +47 B (0%)
build/block-library/index.min.js 145 kB +341 B (0%)
build/block-library/style-rtl.css 10.2 kB +17 B (0%)
build/block-library/style.css 10.2 kB +24 B (0%)
build/block-serialization-default-parser/index.min.js 1.29 kB -1 B (0%)
build/blocks/index.min.js 47.3 kB -6 B (0%)
build/components/index.min.js 182 kB +1.32 kB (+1%)
build/components/style-rtl.css 16.2 kB +26 B (0%)
build/components/style.css 16.2 kB +24 B (0%)
build/core-data/index.min.js 12.4 kB +262 B (+2%)
build/customize-widgets/index.min.js 10.1 kB +164 B (+2%)
build/customize-widgets/style-rtl.css 1.48 kB +29 B (+2%)
build/customize-widgets/style.css 1.48 kB +31 B (+2%)
build/data-controls/index.min.js 828 B +1 B (0%)
build/data/index.min.js 7.23 kB +2 B (0%)
build/date/index.min.js 31.8 kB +1 B (0%)
build/edit-navigation/index.min.js 13.9 kB -69 B (0%)
build/edit-navigation/style-rtl.css 3.12 kB +30 B (+1%)
build/edit-navigation/style.css 3.12 kB +32 B (+1%)
build/edit-post/classic-rtl.css 483 B +29 B (+6%) 🔍
build/edit-post/classic.css 483 B +29 B (+6%) 🔍
build/edit-post/index.min.js 58.7 kB -1 B (0%)
build/edit-post/style-rtl.css 7.29 kB +251 B (+4%)
build/edit-post/style.css 7.28 kB +251 B (+4%)
build/edit-site/index.min.js 25.9 kB -107 B (0%)
build/edit-site/style-rtl.css 4.99 kB +229 B (+5%) 🔍
build/edit-site/style.css 4.98 kB +227 B (+5%) 🔍
build/edit-widgets/index.min.js 16.1 kB +48 B (0%)
build/edit-widgets/style-rtl.css 3.88 kB +318 B (+9%) 🔍
build/edit-widgets/style.css 3.88 kB +319 B (+9%) 🔍
build/editor/index.min.js 38.6 kB +9 B (0%)
build/editor/style-rtl.css 3.94 kB +30 B (+1%)
build/editor/style.css 3.94 kB +31 B (+1%)
build/element/index.min.js 3.44 kB +2 B (0%)
build/format-library/index.min.js 5.69 kB +8 B (0%)
build/format-library/style-rtl.css 668 B +31 B (+5%) 🔍
build/format-library/style.css 669 B +30 B (+5%) 🔍
build/hooks/index.min.js 1.76 kB -1 B (0%)
build/html-entities/index.min.js 628 B -1 B (0%)
build/is-shallow-equal/index.min.js 710 B +1 B (0%)
build/keycodes/index.min.js 1.43 kB -1 B (0%)
build/list-reusable-blocks/index.min.js 2.07 kB -1 B (0%)
build/list-reusable-blocks/style-rtl.css 842 B +213 B (+34%) 🚨
build/list-reusable-blocks/style.css 842 B +214 B (+34%) 🚨
build/notices/index.min.js 1.07 kB -1 B (0%)
build/nux/index.min.js 2.31 kB -1 B (0%)
build/nux/style-rtl.css 745 B +27 B (+4%)
build/nux/style.css 742 B +26 B (+4%)
build/plugins/index.min.js 1.99 kB +1 B (0%)
build/react-i18n/index.min.js 924 B -1 B (0%)
build/reusable-blocks/style-rtl.css 256 B +31 B (+14%) ⚠️
build/reusable-blocks/style.css 256 B +31 B (+14%) ⚠️
build/rich-text/index.min.js 10.6 kB +1 B (0%)
build/server-side-render/index.min.js 1.63 kB -1 B (0%)
build/viewport/index.min.js 1.28 kB +1 B (0%)
build/widgets/index.min.js 6.23 kB +19 B (0%)
build/widgets/style-rtl.css 1 kB +145 B (+17%) ⚠️
build/widgets/style.css 1.01 kB +145 B (+17%) ⚠️
ℹ️ View Unchanged
Filename Size
build/annotations/index.min.js 2.93 kB
build/api-fetch/index.min.js 2.44 kB
build/autop/index.min.js 2.28 kB
build/blob/index.min.js 672 B
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 112 B
build/block-library/blocks/audio/style.css 112 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 475 B
build/block-library/blocks/button/editor.css 474 B
build/block-library/blocks/button/style-rtl.css 603 B
build/block-library/blocks/button/style.css 602 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 375 B
build/block-library/blocks/buttons/style.css 375 B
build/block-library/blocks/calendar/style-rtl.css 208 B
build/block-library/blocks/calendar/style.css 208 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 190 B
build/block-library/blocks/columns/editor.css 190 B
build/block-library/blocks/columns/style-rtl.css 422 B
build/block-library/blocks/columns/style.css 422 B
build/block-library/blocks/cover/editor-rtl.css 670 B
build/block-library/blocks/cover/editor.css 670 B
build/block-library/blocks/cover/style-rtl.css 1.22 kB
build/block-library/blocks/cover/style.css 1.23 kB
build/block-library/blocks/embed/editor-rtl.css 486 B
build/block-library/blocks/embed/editor.css 486 B
build/block-library/blocks/embed/style-rtl.css 401 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 301 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 780 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 704 B
build/block-library/blocks/gallery/editor.css 705 B
build/block-library/blocks/gallery/style-rtl.css 1.06 kB
build/block-library/blocks/gallery/style.css 1.06 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 160 B
build/block-library/blocks/group/editor.css 160 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 93 B
build/block-library/blocks/group/theme.css 93 B
build/block-library/blocks/heading/editor-rtl.css 152 B
build/block-library/blocks/heading/editor.css 152 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 259 B
build/block-library/blocks/home-link/style.css 259 B
build/block-library/blocks/html/editor-rtl.css 281 B
build/block-library/blocks/html/editor.css 281 B
build/block-library/blocks/image/editor-rtl.css 729 B
build/block-library/blocks/image/editor.css 727 B
build/block-library/blocks/image/style-rtl.css 481 B
build/block-library/blocks/image/style.css 485 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-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 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/style-rtl.css 492 B
build/block-library/blocks/media-text/style.css 489 B
build/block-library/blocks/more/editor-rtl.css 434 B
build/block-library/blocks/more/editor.css 434 B
build/block-library/blocks/navigation-link/editor-rtl.css 633 B
build/block-library/blocks/navigation-link/editor.css 634 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation/editor-rtl.css 1.55 kB
build/block-library/blocks/navigation/editor.css 1.55 kB
build/block-library/blocks/navigation/view.min.js 2.87 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 311 B
build/block-library/blocks/page-list/style-rtl.css 240 B
build/block-library/blocks/page-list/style.css 240 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 247 B
build/block-library/blocks/paragraph/style.css 248 B
build/block-library/blocks/post-author/editor-rtl.css 209 B
build/block-library/blocks/post-author/editor.css 209 B
build/block-library/blocks/post-author/style-rtl.css 183 B
build/block-library/blocks/post-author/style.css 184 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 139 B
build/block-library/blocks/post-content/editor.css 139 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 338 B
build/block-library/blocks/post-featured-image/editor.css 338 B
build/block-library/blocks/post-featured-image/style-rtl.css 141 B
build/block-library/blocks/post-featured-image/style.css 141 B
build/block-library/blocks/post-template/editor-rtl.css 100 B
build/block-library/blocks/post-template/editor.css 99 B
build/block-library/blocks/post-template/style-rtl.css 379 B
build/block-library/blocks/post-template/style.css 380 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 183 B
build/block-library/blocks/pullquote/editor.css 183 B
build/block-library/blocks/pullquote/style-rtl.css 318 B
build/block-library/blocks/pullquote/style.css 318 B
build/block-library/blocks/pullquote/theme-rtl.css 169 B
build/block-library/blocks/pullquote/theme.css 169 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 86 B
build/block-library/blocks/query-title/editor.css 86 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 221 B
build/block-library/blocks/quote/theme.css 221 B
build/block-library/blocks/rss/editor-rtl.css 201 B
build/block-library/blocks/rss/editor.css 202 B
build/block-library/blocks/rss/style-rtl.css 290 B
build/block-library/blocks/rss/style.css 290 B
build/block-library/blocks/search/editor-rtl.css 211 B
build/block-library/blocks/search/editor.css 211 B
build/block-library/blocks/search/style-rtl.css 359 B
build/block-library/blocks/search/style.css 362 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 251 B
build/block-library/blocks/separator/style.css 251 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 512 B
build/block-library/blocks/shortcode/editor.css 512 B
build/block-library/blocks/site-logo/style-rtl.css 154 B
build/block-library/blocks/site-logo/style.css 154 B
build/block-library/blocks/site-tagline/editor-rtl.css 87 B
build/block-library/blocks/site-tagline/editor.css 87 B
build/block-library/blocks/site-title/editor-rtl.css 85 B
build/block-library/blocks/site-title/editor.css 85 B
build/block-library/blocks/social-link/editor-rtl.css 164 B
build/block-library/blocks/social-link/editor.css 165 B
build/block-library/blocks/social-links/editor-rtl.css 800 B
build/block-library/blocks/social-links/editor.css 799 B
build/block-library/blocks/social-links/style-rtl.css 1.34 kB
build/block-library/blocks/social-links/style.css 1.34 kB
build/block-library/blocks/spacer/editor-rtl.css 308 B
build/block-library/blocks/spacer/editor.css 308 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 478 B
build/block-library/blocks/table/editor.css 478 B
build/block-library/blocks/table/style-rtl.css 480 B
build/block-library/blocks/table/style.css 480 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/editor-rtl.css 118 B
build/block-library/blocks/tag-cloud/editor.css 118 B
build/block-library/blocks/tag-cloud/style-rtl.css 94 B
build/block-library/blocks/tag-cloud/style.css 94 B
build/block-library/blocks/template-part/editor-rtl.css 551 B
build/block-library/blocks/template-part/editor.css 550 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 569 B
build/block-library/blocks/video/editor.css 570 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/reset-rtl.css 514 B
build/block-library/reset.css 515 B
build/block-library/theme-rtl.css 692 B
build/block-library/theme.css 693 B
build/block-serialization-spec-parser/index.min.js 3.06 kB
build/compose/index.min.js 10.2 kB
build/deprecated/index.min.js 738 B
build/dom-ready/index.min.js 577 B
build/dom/index.min.js 4.62 kB
build/escape-html/index.min.js 739 B
build/i18n/index.min.js 3.73 kB
build/keyboard-shortcuts/index.min.js 1.74 kB
build/media-utils/index.min.js 3.08 kB
build/primitives/index.min.js 1.03 kB
build/priority-queue/index.min.js 791 B
build/redux-routine/index.min.js 2.82 kB
build/reusable-blocks/index.min.js 2.56 kB
build/shortcode/index.min.js 1.68 kB
build/token-list/index.min.js 848 B
build/url/index.min.js 1.95 kB
build/warning/index.min.js 1.13 kB
build/wordcount/index.min.js 1.24 kB

compressed-size-action

@jasmussen
Copy link
Contributor

Thanks so much for trying this. This is what I see:

nav

Outside of the little spacer complexity at the end (where the two first items should be ul wrapped, as should the last one, but not the spacer), from a pure user experience perspective, this feels like the right path forward.

I understand the reluctance, but it is such a key part of the flow that it's crucial to get right. And this is a challenge that will not be unique to the navigation block, it will come up again and again. One incoming example is the table block receiving nesting support. Basic table markup looks like this:

<table>
     <tr>
         <td>The table body</td>
         <td>with two columns</td>
     </tr>
</table>

But when you add a header or footer, the markup changes:

<table>
    <thead>
        <tr>
            <th colspan="2">The table header</th>
        </tr>
    </thead>
    <tbody>
        <tr>
            <td>The table body</td>
            <td>with two columns</td>
        </tr>
    </tbody>
</table>

While the nesting system could replicate that 1:1, the user experience as it exists today, with each layer seletable, it would be unwieldy. This is an aspect where our friends at TinyMCE do it so well:

tiny tables

In other words, it's a problem we need to solve. In the case of the table block, a passthrough container block as discussed in #7694 might be a key ingredient in flattening the structure and showing a single block toolbar.

In the case of the navigation block, the challenge is that the ul needs to show up conditionally based on the content of your block. And to enable menus that have, for example, 2 menu items, a site logo, and then another 2 menu items, this is a challenge that needs solving. The missing UL in the editor is a tradeoff that feels worth it to me, and something I can help CSS up easily.

@youknowriad I'd love your thoughts here, as I know you've been looking at conditional divs, serverside rendered, lately.

@jasmussen
Copy link
Contributor

Coming back to this after thinking about it more, I think the most important bit of all is that we should not put the onus on the user to figure out what type of blocks need be wrapped in what containers. Users see a navigation block and assume they can insert links to pages, perhaps search, perhaps social links.

When framed like that, what gets output on the frontend then becomes our headache to solve, either through server side rendering like in this PR, or revisiting the structure of the markup. With the server side rendering, the trade-off of markup that diverges between editor and frontend is easily worth it, as we can maintain similar classnames to target instead of the elements directly.

CC: @noisysocks @priethor @mtias @draganescu

@draganescu
Copy link
Contributor

I still feel like having unsemantic html on the editor side should not be used as an escape hatch.

We should use this solution only if we arrive to the conclusion that it is impossible to have a good UX and semantic editor markup. For now it's just complicated to see the best solution but not impossible.

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Great exploration @tellthemachines ! ✨

My overall impression is that we do want the overall behavior of this branch (in terms of user interactions), but two things might make us hesitate on this implementation approach:

This definitely works, but if we end up choosing this approach, it'd be more of a necessary one-off since it's specific to the navigation block. We'd likely end up revisiting the problem for other wrapper blocks.

For next steps, I suspect we'll need to explore #30430, and if we hit a really bad blocker, we can double back to this approach

if ($inner_block->name !== "core/navigation-link" && $inner_block->name !== "core/spacer" && $is_list_open === true) {
$is_list_open = false;
$inner_blocks_html .= '</ul>';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, much closer to expected html markup. If we go with this approach, children of ul, still need to be wrapped in an li. (For example the spacer div).

Screen Shot 2021-04-26 at 8 59 14 AM

@tellthemachines
Copy link
Contributor Author

The discussion in #31951 points to this solution as the best way forward, so I'm marking this PR ready for review now.

I've also fixed some conflicts and updated this PR to work with the responsive wrapper.

@tellthemachines tellthemachines marked this pull request as ready for review May 28, 2021 06:44
@ZebulanStanphill ZebulanStanphill added the [Block] Navigation Affects the Navigation Block label Jun 4, 2021
@tellthemachines
Copy link
Contributor Author

Updated this PR to also remove the li and uls from Navigation Link block on the editor. Front end output remains unchanged.

I've also excluded the Spacer block from the uls, and temporarily removed it from the list of allowed children of Navigation Link as we cannot semantically include it until we make changes to how Spacer renders. I'll be experimenting with how to solve that problem in the next few days, but it should be a separate PR.

Reviews and feedback appreciated!

@jasmussen
Copy link
Contributor

Took it for a spin:

nav

As far as I can tell, this one threads the needle! Very impressive work 👌

@@ -48,7 +48,7 @@ import { store as coreStore } from '@wordpress/core-data';
import { ItemSubmenuIcon } from './icons';
import { name } from './block.json';

const ALLOWED_BLOCKS = [ 'core/navigation-link', 'core/spacer' ];
const ALLOWED_BLOCKS = [ 'core/navigation-link' ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we mean to remove the spacer from being insertable from submenus? We could maybe leave it for a follow up, it might be possible to do something like modify padding on the items instead of inserting an element for the published view.

Screen Shot 2021-06-25 at 10 46 07 AM

As a note for the other reviewers, existing menus with a spacer still appear, but new menus won't allow spacers to be inserted in sub-menus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it temporarily, as there are a couple of things we need to fix for it to work properly in submenus:

  • the markup issue (ideally by getting the spacer to not render markup at all, or else by wrapping it in an li)
  • making the spacer vertical within the submenu of a horizontal nav (right now it only works vertically within vertical nav blocks)

Copy link
Contributor Author

@tellthemachines tellthemachines Jun 28, 2021

Choose a reason for hiding this comment

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

Update: I created #33018 to discuss the best way forward for the Spacer markup issue, and there is #30590 for its behaviour in horizontal Nav submenus. I think we can fix these separately from this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

At some "phase 2" of the block, we'll want to tackle mega-menus, where the contents of a dropdown are virtually as flexible as what you can insert in a group block, so it's something we should come back to. But definitely fine to wrap a solid V1 first.

@gwwar
Copy link
Contributor

gwwar commented Jun 25, 2021

Overall this looks pretty technically sound to me, and the user experience here feels good! I smoke tested a bit, and couldn't find any issues.

In terms of moving this forward:

  • Let's double check that folks were okay with the editor/published view . It feels that we are, I just wanted us to mark that down as a comment on the PR.
  • 🔍 if there are any other styling changes needed.

@draganescu
Copy link
Contributor

@gwwar what does this mean:

Let's double check that folks were okay with the editor/published view .

@gwwar
Copy link
Contributor

gwwar commented Jun 28, 2021

@gwwar what does this mean:

That we were okay with diverging editor markup from published markup. This is mostly to document for other 👀 since I don't expect folks to be aware of all the conversation threads here on it.

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 looks good, let's :shipit:

@draganescu draganescu merged commit 4de53d5 into trunk Jul 7, 2021
@draganescu draganescu deleted the try/nav-list-markup branch July 7, 2021 03:28
@github-actions github-actions bot added this to the Gutenberg 11.1 milestone Jul 7, 2021
@annezazu annezazu added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Oct 5, 2021
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 [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants