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

Add support for responsive navigation block in Gutenberg #6319

Closed
westonruter opened this issue May 28, 2021 · 8 comments · Fixed by #6838
Closed

Add support for responsive navigation block in Gutenberg #6319

westonruter opened this issue May 28, 2021 · 8 comments · Fixed by #6838
Assignees
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one Integration: Gutenberg P1 Medium priority
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented May 28, 2021

Feature description

As shown in What’s new in Gutenberg 10.7? (26 May), there is a new Responsive Navigation block:

image

This opens a sidebar on a narrow viewport:

Closed Open
image image

We can implement support for this via amp-sidebar.

When the block has responsive menus enabled, it enqueues the frontend.js script which achieves the sidebar modal via the Micromodal library.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

  1. Add a render_block filter for the navigation block. See AMP_Core_Block_Handler.
  2. Check if the isResponsive is set, and if not, short-circuit.
  3. Dequeue wp-block-navigation-view .
  4. (TBD) Wrap the nav menu in amp-sidebar and amend the button with the necessary action to toggle whether the menu is sidebar is opened.

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added the Enhancement New feature or improvement of an existing one label May 28, 2021
@westonruter westonruter added this to the v2.2 milestone May 28, 2021
@westonruter westonruter added the P1 Medium priority label Oct 25, 2021
@westonruter
Copy link
Member Author

When the Twenty Twenty-Two theme active, two validation errors are currently being emitted because of this:

image

@westonruter
Copy link
Member Author

Note: Instead of a render_block filter to the AMP_Core_Block_Handler class (which will entail manipulating an HTML string), it may make sense instead to utilize a sanitizer instead where the DOM can be interacted with instead. If the render_block filter is clean, then we can go ahead with that.

@westonruter
Copy link
Member Author

If it ends up as being that aria-modal needs to be used in the AMP version, I've opened ampproject/amphtml#37002 to recognize that missing attribute in the validator spec.

@bartoszgadomski
Copy link
Contributor

bartoszgadomski commented Nov 24, 2021

After digging a bit into this I think we could use <amp-lightbox> instead of <amp-sidebar>, because the lightbox element introduces less CSS styles, so we wouldn't have to overwrite them later to achieve the menu styled in the same way as in "non-amp" version.

The original markup of wp-block-navigation looks like this:

<nav class="wp-container-619eab71221c9 is-responsive wp-block-navigation">
    <button aria-expanded="false" aria-haspopup="true" aria-label="Open menu" class="wp-block-navigation__responsive-container-open" data-micromodal-trigger="modal-619eab7121f8f">
        <svg width="24" height="24" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" role="img" aria-hidden="true" focusable="false">
            <rect x="4" y="7.5" width="16" height="1.5" />
            <rect x="4" y="15" width="16" height="1.5" />
        </svg>
    </button>
    <div class="wp-block-navigation__responsive-container" id="modal-619eab7121f8f">
        <div class="wp-block-navigation__responsive-close" tabindex="-1" data-micromodal-close>
            <div class="wp-block-navigation__responsive-dialog" role="dialog" aria-modal="true" aria-labelledby="modal-619eab7121f8f-title">
                <button aria-label="Close menu" data-micromodal-close class="wp-block-navigation__responsive-container-close">
                    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" role="img" aria-hidden="true" focusable="false">
                        <path d="M13 11.8l6.1-6.3-1-1-6.1 6.2-6.1-6.2-1 1 6.1 6.3-6.5 6.7 1 1 6.5-6.6 6.5 6.6 1-1z"></path>
                    </svg>
                </button>
                <div class="wp-block-navigation__responsive-container-content" id="modal-619eab7121f8f-content">
                    <ul class="wp-block-page-list">
                        <li class="wp-block-pages-list__item wp-block-navigation-item open-on-hover-click">
                            <a class="wp-block-pages-list__item__link wp-block-navigation-item__content" href="http://localhost.develop/sample-page/">Sample Page</a>
                        </li>
                    </ul>
                </div>
            </div>
        </div>
    </div>
</nav>

We could do following changes to ampify it:

  • "fake" the "open" state by adding is-menu-open has-modal-open classes to div.wp-block-navigation__responsive-container
  • add on="tap:menuX.open" to button.wp-block-navigation__responsive-container-open element
  • add on="tap:menuX.close" to button.wp-block-navigation__responsive-container-close element
  • wrap div.wp-block-navigation__responsive-container with <amp-lightbox id="menuX" layout="nodisplay">...</amp-lightbox>
  • remove data-micromodal-trigger and data-micromodal-close attributes
  • remove aria-expanded and aria-modal attributes
  • duplicate div.wp-block-navigation__responsive-container (original one, without extra classes) outside the amp-lightbox wrapper
  • dequeue the wp-block-navigation-view script

so the final markup will look similar to this:

<nav class="wp-container-619e2a8d594e4 is-responsive wp-block-navigation">
    <button on="tap:menu1.open" aria-haspopup="true" aria-label="Open menu" class="wp-block-navigation__responsive-container-open">
        <svg width="24" height="24" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" role="img" aria-hidden="true" focusable="false">
            <rect x="4" y="7.5" width="16" height="1.5"></rect>
            <rect x="4" y="15" width="16" height="1.5"></rect>
        </svg>
    </button>
    <amp-lightbox id="menu1" layout="nodisplay">
        <div class="wp-block-navigation__responsive-container is-menu-open has-modal-open" id="modal-619e2a8d5925c">
            <div class="wp-block-navigation__responsive-close" tabindex="-1">
                <div class="wp-block-navigation__responsive-dialog" role="dialog" aria-labelledby="modal-619e2a8d5925c-title">
                    <button on="tap:menu1.close" aria-label="Close menu" class="wp-block-navigation__responsive-container-close">
                        <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" role="img" aria-hidden="true" focusable="false">
                            <path d="M13 11.8l6.1-6.3-1-1-6.1 6.2-6.1-6.2-1 1 6.1 6.3-6.5 6.7 1 1 6.5-6.6 6.5 6.6 1-1z"></path>
                        </svg>
                    </button>
                    <div class="wp-block-navigation__responsive-container-content" id="modal-619e2a8d5925c-content">
                        <ul class="wp-block-page-list">
                            <li class="wp-block-pages-list__item wp-block-navigation-item open-on-hover-click">
                                <a class="wp-block-pages-list__item__link wp-block-navigation-item__content" href="http://localhost.develop/sample-page/">Sample Page</a>
                            </li>
                        </ul>
                    </div>
                </div>
            </div>
        </div>
    </amp-lightbox>
    <div class="wp-block-navigation__responsive-container">
        <div class="wp-block-navigation__responsive-container-content">
            <ul class="wp-block-page-list">
                <li class="wp-block-pages-list__item wp-block-navigation-item open-on-hover-click">
                    <a class="wp-block-pages-list__item__link wp-block-navigation-item__content" href="http://localhost.develop/sample-page/">Sample Page</a>
                </li>
            </ul>
        </div>
    </div>
</nav>

Thanks to this, we could use the same navigation menu styles as were used in original, non-amp version of the page, while getting rid of the original JS scripts.

@westonruter
Copy link
Member Author

Since WordPress 5.9 has been delayed until January, I'm going to bump this to the 2.3 milestone.

@bartoszgadomski
Copy link
Contributor

During further investigation I've found that one more thing needs to be addressed: user can choose to open submenu items "on click". When li.open-on-click button.wp-block-navigation-submenu__toggle element is clicked, the aria-expanded attribute changes from false to true (JS script does that change), and accompanying submenu is then displayed to the user.

To ampify it, I used the <amp-state> element and integrated it with toggle buttons.

@fellyph
Copy link
Collaborator

fellyph commented Jan 28, 2022

Hi @westonruter,

I have tested on the developer Branch and I was QA passed

@fellyph
Copy link
Collaborator

fellyph commented Jan 28, 2022

Tested at 2.2.x on Chrome and Safari and QA Passed

Chrome
Screen Shot 2022-01-28 at 19 03 45

Screen Shot 2022-01-28 at 19 03 59

Safari
IMG_8822
IMG_8821

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one Integration: Gutenberg P1 Medium priority
Projects
None yet
5 participants