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.

Sign up for GitHub

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

Jump to bottom

Update <amp-nested-menu> event listeners and improve RTL + arrow key support #25277

Merged
merged 3 commits into from Oct 29, 2019
Merged

Update <amp-nested-menu> event listeners and improve RTL + arrow key support #25277

merged 3 commits into from Oct 29, 2019

Conversation

leafsy
Copy link
Contributor

@leafsy leafsy commented Oct 28, 2019

Resolves: #25157
Resolves: #24665

Implement the following changes to <amp-nested-menu>:

  • Add an optional "side" attribute that determines which side submenus should open from ("right" by default, "left" for RTL documents)
  • Support all arrow keys:
    • Left arrow (or right if side=left) to go back to parent menu;
    • Right arrow (or left if side=right) to open submenu if submenu open button has focus;
    • Up/Down arrow to move focus within menu (only works for <li> in the same list);
  • Add a public reset() method that sidebar can call to reset menus on close;
  • Change event listeners to use event delegation;
  • Add tests for the new functionalities.

Demo: https://amp-nested-menu.firebaseapp.com/

@leafsy leafsy merged commit bf12b35 into ampproject:master Oct 29, 2019
@leafsy leafsy deleted the nested-menu-fixes branch October 29, 2019 21:24
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
@leafsy
Update <amp-nested-menu> event listeners and improve RTL + arrow key …
50a5d54 
…support (ampproject#25277)

* update nested menu with rtl and key support
* update nested menu tests
* rework close button check logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cathyxz cathyxz cathyxz approved these changes

@sparhami sparhami Awaiting requested review from sparhami

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support RTL for amp-nested-menu Improve keyboard support for <amp-nested-menu>
3 participants
@leafsy @cathyxz @googlebot