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

Use amp-nested-menu to implement mobile menu in Twenty Nineteen #4400

Closed
westonruter opened this issue Mar 17, 2020 · 3 comments · Fixed by #6660
Closed

Use amp-nested-menu to implement mobile menu in Twenty Nineteen #4400

westonruter opened this issue Mar 17, 2020 · 3 comments · Fixed by #6660
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one Groomed Integration: WP Core P1 Medium priority RME Reader Mode Expansion WS:UX Work stream for UX/Front-end
Milestone

Comments

@westonruter
Copy link
Member

Feature description

When AMP compatibility was added for the Twenty Nineteen theme, we tried our best to implement support for the mobile menu (or rather, to use CSS only to implement the feature: WordPress/twentynineteen#337):

Comps for submenu

However, we weren't able to implement this without JS (see touch-keyboard-navigation.js), so we had to ship the no-JS fallback experience for AMP (#1587).

However, there is now an AMP component dedicated for this purpose: amp-nested-menu. We should now leverage it in the core theme sanitizer.


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

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added Enhancement New feature or improvement of an existing one Integration: WP Core labels Mar 17, 2020
@westonruter westonruter added this to the v1.6 milestone Mar 17, 2020
@westonruter westonruter added RME Reader Mode Expansion P1 Medium priority labels Apr 12, 2020
@westonruter
Copy link
Member Author

Marking this as P1 since we'll be using Twenty Nineteen potentially as one of the templates we make available in Reader mode.

@kmyram kmyram modified the milestones: v1.6, Sprint 28 Apr 14, 2020
@kmyram kmyram modified the milestones: Sprint 28, v1.6 May 27, 2020
@westonruter westonruter removed this from the v1.6 milestone Jun 15, 2020
@kmyram kmyram added the Groomed label Jun 30, 2020
@kmyram kmyram added the WS:UX Work stream for UX/Front-end label Aug 5, 2020
@westonruter westonruter added this to the v2.2 milestone Feb 11, 2021
@pierlon pierlon self-assigned this Jul 14, 2021
@dhaval-parekh dhaval-parekh self-assigned this Oct 11, 2021
@westonruter westonruter modified the milestones: v2.2, v2.3 Dec 7, 2021
@westonruter
Copy link
Member Author

The Bento version of amp-sidebar is incompatible with non-Bento amp-nested-menu. Since there is no Bento version of amp-nested-menu yet, this issue is blocked.

@westonruter westonruter removed this from the v2.3 milestone Jan 13, 2022
@westonruter westonruter added this to the v2.3.1 milestone Sep 29, 2022
@pavanpatil1
Copy link

QA Passed ✅

Cross-checked the issue, Fix is working as expected.

Non AMP

navnonamp.mp4

AMP

navamp.mp4

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023
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 Groomed Integration: WP Core P1 Medium priority RME Reader Mode Expansion WS:UX Work stream for UX/Front-end
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants