Add validation and documentation for <amp-mega-menu> by leafsy · Pull Request #25210 · ampproject/amphtml

leafsy · 2019-10-22T22:56:08Z

Add validation and test files for the <amp-mega-menu> extension.

The following items are enforced via validation:

Whitelist the components below inside mega menu:
- <amp-list>
- <amp-form>
- <amp-img>
- <amp-video>
- <amp-carousel>
- <amp-lightbox>
- <amp-ad>
Mega menu should contain a single nav element, which:
- Has either ul or ol as its only child;
- Has no other siblings;
- Is parented by either mega menu itself or amp-list/template;
The ul/ol under nav can contain only li (menu items) as children;
Every menu item should have 1~2 direct children:
- If there are two children, exactly one should have role="dialog";
- Disallow role=”menu” as a child (until supported by implementation);

The change in validator-main.protoascii was made so that nav inside amp-mega-menu does not match to the general nav tag in this file.

PR Deployed Example (need the amp-mega-menu experiment on)

amp-owners-bot · 2019-10-22T22:56:19Z

Hey @ampproject/wg-caching, these files were changed:

extensions/amp-mega-menu/0.1/test/validator-amp-mega-menu-error.html
extensions/amp-mega-menu/0.1/test/validator-amp-mega-menu-error.out
extensions/amp-mega-menu/0.1/test/validator-amp-mega-menu.html
extensions/amp-mega-menu/0.1/test/validator-amp-mega-menu.out
extensions/amp-mega-menu/validator-amp-mega-menu.protoascii

cathyxz · 2019-10-22T23:14:42Z

Oh nvm I see that owners bot has been smart enough to tag @ampproject/wg-caching already. =)

cathyxz

I'd defer to @ampproject/wg-caching for the correct usage of reference points, but this looks fine. Consider adding your documentation to this PR, since we should review to make sure that validator rules and documentation are consistent with each other.

cathyxz · 2019-10-22T23:08:29Z

extensions/amp-mega-menu/validator-amp-mega-menu.protoascii

+  tag_name: "AMP-MEGA-MENU"
+  requires_extension: "amp-mega-menu"
+  # Mandate that component contains a nav element as descendant. This approach
+  # only works because there is at most one <amp-mega-menu> in a document.


/fyi @nainar this is a hard restriction that at most one mega menu is allowed per document.

extensions/amp-mega-menu/validator-amp-mega-menu.protoascii

extensions/amp-mega-menu/0.1/test/validator-amp-mega-menu.html

cathyxz · 2019-10-23T23:41:18Z

extensions/amp-mega-menu/amp-mega-menu.md

@@ -0,0 +1,228 @@
+---


/fyi @nainar in case you would like to take a look.
/cc @CrystalOnScript for a documentation review

extensions/amp-mega-menu/amp-mega-menu.md

cathyxz · 2019-10-23T23:52:07Z

extensions/amp-mega-menu/amp-mega-menu.md

+    <td>This element includes <a href="https://amp.dev/documentation/guides-and-tutorials/learn/common_attributes">common attributes</a> extended to AMP components.</td>
+  </tr>
+</table>
+


Add a section on Accessibility. In particular, reference the accessibility documentation with regards to Hover, as well as explain the elements and attributes added for accessibility.

cathyxz

LGTM

honeybadgerdontcare · 2019-10-24T21:19:14Z

@twifkak for detailed validation review. I've requested @leafsy to split the validator test file into separate test files since it's usage is unique and then any <!-- Valid: ...> will return a PASS for that file.

CrystalOnScript

Had some clarity questions and structural changes - but overall looking good! Will do another passthrough when changes are made :)

CrystalOnScript · 2019-10-26T21:34:31Z

extensions/amp-mega-menu/amp-mega-menu.md

+  </tr>
+</table>
+
+## Overview


Overall this looks good! We're attempting to standardize the reference documentation - theres a template that outlines headers and content here. Would you mind taking a look a seeing if theres any updates we can make to this document to align with the template? Please let me know if theres anything confusing or if you disagree with anything in the template. Its a WIP :)

Thanks for putting the reference template together! Some take aways & updates I'll make are:

Take the description <tr> out of the table and move its content to immediately after the extension title.

Remove the examples <tr>.

Move the accessibility section to just before validation.

Add an example for styling the component.

extensions/amp-mega-menu/amp-mega-menu.md

leafsy · 2019-10-28T05:45:59Z

@CrystalOnScript Really appreciate your review! It's my first time writing this kind of documentation, so definitely nice to get more eyes on the pr.

About the restriction of one <amp-mega-menu> per document, I've given it more thought and think that we should drop it from validation rules. I can think of use cases where developer might include multiple mega menus for different devices and switch them via media query. What we don't want is nested mega menus, which should already been prevented via validation. /cc @nainar on this

CrystalOnScript

Did a much heavier style/content review - let me know if you have questions/concerns!

extensions/amp-mega-menu/amp-mega-menu.md

CrystalOnScript

Looking really great! Just some last minute nits I didn't catch before :)

CrystalOnScript · 2019-10-29T18:13:18Z

extensions/amp-mega-menu/amp-mega-menu.md

+
+# `amp-mega-menu`
+
+A horizontal navigation bar containing a set of menu items that, when activated via click, open/close corresponding containers underneath with additional content.


Simplify to:

Horizontal navigation bar with menu items that open/close content containers on click.

CrystalOnScript · 2019-10-29T18:17:50Z

extensions/amp-mega-menu/amp-mega-menu.md

+
+## Overview
+
+`<amp-mega-menu>` provides a way to organize and display large collections of navigational content at the top of an AMP page. The component is intended primarily for desktop and tablet use cases, and it can be used jointly with [`<amp-sidebar>`](../amp-sidebar/0.1/amp-sidebar.md) to create a responsive menu.


I'd rewrite as:

The <amp-mega-menu> component includes a single
element containing either a <ul> or <ol>, where each <li> element is a menu item.

CrystalOnScript · 2019-10-29T18:19:37Z

extensions/amp-mega-menu/amp-mega-menu.md

+
+### Dynamic content rendering
+
+Fetch content of `<amp-mega-menu>` dynamically from a JSON endpoint using [`<amp-list>`](../amp-list/amp-list.md) and [`amp-mustache`](../amp-mustache/amp-mustache.md) template.


Wrap amp-mustache in <>

CrystalOnScript · 2019-10-29T18:21:27Z

extensions/amp-mega-menu/amp-mega-menu.md

+
+The example below customizes:
+
+- The background color of the navigation bar


Mind adding a . at the end of each bullet?

CrystalOnScript · 2019-10-29T18:21:56Z

extensions/amp-mega-menu/amp-mega-menu.md

+
+Keyboard support for the component includes:
+
+- Left/right arrow keys to navigate between menu items when focused;


Can you update the ; to .?

twifkak · 2019-10-30T20:28:18Z

validator/validator-main.protoascii

  html_format: AMP4EMAIL
  html_format: ACTIONS
  tag_name: "NAV"
+  disallowed_ancestor: "AMP-MEGA-MENU"


Add a comment that inside AMP-MEGA-MENU, this is replaced by more specific rules. (It took me a while to figure out why this was here.)

twifkak · 2019-10-30T21:25:54Z

extensions/amp-mega-menu/validator-amp-mega-menu.protoascii

+  child_tags: {
+    mandatory_num_child_tags: 1
+    child_tag_name_oneof: "NAV"
+    child_tag_name_oneof: "AMP-LIST"


Can you add a test for this? amp-list does not satisfy "amp-mega-menu" so given the mandatory_num_child_tags: 1, I don't see how this can be used (unless you have two mega-menus as I mention above).

So my intention is that mega menu can follow one of two patterns:
<amp-mega-menu> > <nav> > ... or
<amp-mega-menu> > <amp-list> > <template> > <nav> > ...
Where the nav underneath <template> would satisfy the requirement. Examples of both cases are included in validator-amp-mega-menu.html.

twifkak · 2019-10-30T21:27:43Z

extensions/amp-mega-menu/validator-amp-mega-menu.protoascii

+  html_format: AMP
+  tag_name: "AMP-MEGA-MENU"
+  requires_extension: "amp-mega-menu"
+  requires: "amp-mega-menu nav"


requires/satisfies may not be doing exactly what you expect. They are whole-document booleans. e.g. I think this will be valid:

<amp-mega-menu layout=fixed-height> <amp-list></amp-list>  </amp-mega-menu> <amp-mega-menu layout=fixed-height> <nav><ol>...valid li...</ol></nav>  </amp-mega-menu>

If I'm wrong about that, add a test to prove me wrong. :)

But is this necessary? Maybe the mandatory_num_child_tags+child_tag_name_oneof is sufficient?

Ah you're right. It'd be simple if we didn't need to support the second case in the comment above. Rather than using requires/satisfies, perhaps I can still use reference points to cover both cases.

@twifkak updated the validation rules and added some more tests. I'm now using reference points to validate the two cases mentioned before, and should no longer need to change validator-main.protoascii. The only situation not covered by the new rules is nested navs, which I can add as a dev warning instead.

extensions/amp-mega-menu/validator-amp-mega-menu.protoascii

CrystalOnScript

Thanks for all the edits! LGTM :)

twifkak · 2019-10-31T21:23:31Z

extensions/amp-mega-menu/validator-amp-mega-menu.protoascii

-  siblings_disallowed: true
+  tag_name: "$REFERENCE_POINT"
+  spec_name: "AMP-MEGA-MENU > AMP-LIST"
+  # Include mandatory amp-list attrs so that <amp-mega-menu> > <nav>


Neat hack. We're lucky that the two can be differentiated by attributes. It seems a bit fragile given it's dependent on the mandatory_anyof in validator-amp-list.protoascii (or else a no-attrs <amp-list/> would match the nav ref point), but I can't think of a better solution off the top of my head.

Indeed; it'd be nice if we could distinguish reference points by tag name, but not sure if there's significant engineering cost associated with that.

* cl/277925913 Revision bump for #25291 * cl/277925913 Revision bump for #25291 * cl/278402306 Remove unused CSS selector code * cl/278620720 Revision bump for #25210 * cl/278642597 Revision bump for #24793 * cl/278653831 Revision bump for #25425 * Fix merge issues in original rollup commit

* add validation files * add documentations * add example file

* cl/277925913 Revision bump for ampproject#25291 * cl/277925913 Revision bump for ampproject#25291 * cl/278402306 Remove unused CSS selector code * cl/278620720 Revision bump for ampproject#25210 * cl/278642597 Revision bump for ampproject#24793 * cl/278653831 Revision bump for ampproject#25425 * Fix merge issues in original rollup commit

googlebot added the cla: yes label Oct 22, 2019

amp-owners-bot bot requested a review from amaltas October 22, 2019 22:56

leafsy requested a review from cathyxz October 22, 2019 22:58

cathyxz requested review from Gregable and removed request for Gregable October 22, 2019 23:14

cathyxz reviewed Oct 22, 2019

View reviewed changes

honeybadgerdontcare reviewed Oct 23, 2019

View reviewed changes

extensions/amp-mega-menu/0.1/test/validator-amp-mega-menu.html Outdated Show resolved Hide resolved

honeybadgerdontcare requested a review from twifkak October 23, 2019 17:38

leafsy mentioned this pull request Oct 23, 2019

🐛 Update event listeners for mega menu #25091

Merged

leafsy added 3 commits October 23, 2019 15:28

add validation files

55c6eb9

add documentations

bb54a21

add example file

1072e36

leafsy force-pushed the mega-menu-docs branch from f182fba to 1072e36 Compare October 23, 2019 23:32

leafsy changed the title ~~Add validation rules for <amp-mega-menu>~~ Add validation and documentation for <amp-mega-menu> Oct 23, 2019

cathyxz reviewed Oct 23, 2019

View reviewed changes

cathyxz suggested changes Oct 23, 2019

View reviewed changes

update documentation with a11y

dc168da

cathyxz approved these changes Oct 24, 2019

View reviewed changes

cathyxz requested a review from honeybadgerdontcare October 24, 2019 03:52

CrystalOnScript requested changes Oct 26, 2019

View reviewed changes

leafsy force-pushed the mega-menu-docs branch from 1bc2a63 to d530e66 Compare October 28, 2019 20:54

update documentation and separate validation tests

05adc2e

leafsy force-pushed the mega-menu-docs branch from d530e66 to 05adc2e Compare October 28, 2019 21:01

leafsy requested a review from CrystalOnScript October 28, 2019 21:55

CrystalOnScript requested changes Oct 28, 2019

View reviewed changes

another doc update

5128b1f

leafsy requested a review from CrystalOnScript October 29, 2019 00:43

fix prettify on docs

6aaba3d

CrystalOnScript requested changes Oct 29, 2019

View reviewed changes

small doc fixes

71331d2

leafsy requested a review from CrystalOnScript October 30, 2019 16:50

twifkak requested changes Oct 30, 2019

View reviewed changes

CrystalOnScript approved these changes Oct 30, 2019

View reviewed changes

make validator changes

5d1a3f1

twifkak approved these changes Oct 31, 2019

View reviewed changes

leafsy merged commit bab8868 into ampproject:master Oct 31, 2019

leafsy deleted the mega-menu-docs branch October 31, 2019 23:24

amaltas added a commit that referenced this pull request Nov 5, 2019

cl/278620720 Revision bump for #25210

7f3b1c7

cathyxz mentioned this pull request Nov 7, 2019

Launch amp-mega-menu to 100% of canary and production #25480

Merged

micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019

Add validation and documentation for <amp-mega-menu> (ampproject#25210)

1726dba

* add validation files * add documentations * add example file


		# `amp-mega-menu`

		A horizontal navigation bar containing a set of menu items that, when activated via click, open/close corresponding containers underneath with additional content.


		## Overview

		`<amp-mega-menu>` provides a way to organize and display large collections of navigational content at the top of an AMP page. The component is intended primarily for desktop and tablet use cases, and it can be used jointly with [`<amp-sidebar>`](../amp-sidebar/0.1/amp-sidebar.md) to create a responsive menu.


		### Dynamic content rendering

		Fetch content of `<amp-mega-menu>` dynamically from a JSON endpoint using [`<amp-list>`](../amp-list/amp-list.md) and [`amp-mustache`](../amp-mustache/amp-mustache.md) template.


		The example below customizes:

		- The background color of the navigation bar


		Keyboard support for the component includes:

		- Left/right arrow keys to navigate between menu items when focused;

Conversation

leafsy commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amp-owners-bot bot commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cathyxz commented Oct 22, 2019

Uh oh!

cathyxz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cathyxz left a comment

Choose a reason for hiding this comment

Uh oh!

honeybadgerdontcare commented Oct 24, 2019

Uh oh!

CrystalOnScript left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leafsy commented Oct 28, 2019

Uh oh!

CrystalOnScript left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CrystalOnScript left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

leafsy commented Oct 22, 2019 •

edited

Loading

amp-owners-bot bot commented Oct 22, 2019 •

edited

Loading

twifkak Oct 31, 2019 •

edited

Loading