Add support of nested AMP navigation for the main menu in twenty nineteen theme. #6660
Conversation
|
Plugin builds for 74c408a are ready 🛎️!
|
There was a problem hiding this comment.
Impressive how closely you've got it looking and behaving like non-AMP! 👍
The only thing I see you couldn't replicate is the "priority+" menu, which we know we couldn't do.
The one challenge I see with the submenus right now as implemented here is tapping them to open.
| Non-AMP | AMP |
|---|---|
 |
 |
Technically the non-AMP priority+ ... button is a bit smaller than the AMP 🔽. Nevertheless, in the latter there is a but more left margin making it easier to tap as opposed to tapping the top-level menu link (I think).
Maybe all of the buttons to open the sidebars should have the same … styling with left margin?
| * | ||
| * @return bool True if request is from mobile device Otherwise False. | ||
| */ | ||
| public static function is_mobile_request() { |
There was a problem hiding this comment.
Is there a way to implement this without resorting to checking the user agent? I worry that checking for whether it is a mobile user agent will not work reliably due to caching layers. This is the same reason why we do client-side redirection by default with JS instead of server-side redirection in PHP.
One possibility would be to use the approach taken for Twenty Seventeen for the sticky menu, where the sanitizer makes a copy of the menu which is shown when it is made sticky. But that may be overkill. It may be worthwhile for each submenu however: include the original li > ul.sub-menu but only allow it to be displayed on desktop. Then also include the li > amp-sidebar but only show it on mobile. This may involve having two buttons, one that is desktop-specific and one that is mobile-specific.
There was a problem hiding this comment.
I have updated the implementation and removed the usage of is_mobile_request() and used the media query to display AMP navigation for mobile.
However, when bento is enabled ( with the filter add_filter( 'amp_bento_enabled', '__return_true' ); ) AMP navigation is broken. Currenttly fixing AMP navigation with bento enabled.
There was a problem hiding this comment.
| Bento disabled | Bento enabled |
|---|---|
 |
 |
I think it is breaking because in bento mode. we are loading script 1.0 version for amp-sidebar component. and that may not support amp-nested-menu
There was a problem hiding this comment.
The issue is caused because the bento component for amp-nested-menu is not available.
…theme Apply suggestions from PR
…h css media query
|
Since we're just a couple days prior to release, I'm going to punt this to 2.3 to go along with the other theme compatibility improvements to go with WordPress 5.9. |
|
@westonruter This is ready for your review. After testing all changes looks good to me. Non-AMP Screencast.from.09-30-2022.09.23.33.PM.webmAMP Screencast.from.09-30-2022.09.27.46.PM.webm |
|
It looks like an E2E test is failing due to a change in Twenty Nineteen: |
Co-authored-by: Weston Ruter <westonruter@google.com>
thelovekesh
force-pushed
the
enhancement/4400-twentynineteen-mobile-menu
branch
from
9ab206a to
74c408a
Compare
| await expect( menuItemWithSubmenu ).toMatchElement( '.sub-menu', { visible: false } ); | ||
|
|
||
| await expect( menuItemWithSubmenu ).toClick( '.submenu-expand' ); | ||
| await expect( menuItemWithSubmenu ).toClick( '.display-on-mobile' ); |
There was a problem hiding this comment.
Was this a change in the theme?
There was a problem hiding this comment.
Nope, it was from core theme sanitizer.
westonruter
added
the
Changelogged
Summary
Fixes #4400
This PR makes the main navigation menu of twenty nineteen theme AMP-compatible. It transforms all first menu items with submenu into AMP nested menu within the AMP sidebar.
None AMP
4400-none-AMP.mov
AMP
4400-AMP.mov
Checklist