Skip to content

Add validation and documentation for <amp-nested-menu> #25339

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
Merged
merged 6 commits into from
Nov 8, 2019
Merged

Add validation and documentation for <amp-nested-menu> #25339

merged 6 commits into from
Nov 8, 2019

Conversation

leafsy
Copy link
Contributor

@leafsy leafsy commented Oct 30, 2019 •
edited
Loading

I2I: #25049

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

The following items are enforced via validation:

  • Nested menu must be inside <amp-sidebar>.
  • Support only layout=fill.
  • Whitelist the components below inside mega menu:
    • <amp-list>
    • <amp-accordion>
    • <amp-img>
  • Each element inside the component may have at most one of the following attributes:
    • amp-nested-submenu: can be applied to div's only, and cannot be the descendant of <amp-accordion>
    • amp-nested-submenu-open: can be applied to div, span, button or heading tags
    • amp-nested-submenu-close: same as above

/cc @nainar we decided to not support ads for now since according to documentation it's not among the allowed components inside sidebar.

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

@amp-owners-bot
Copy link

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

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

CrystalOnScript
CrystalOnScript requested changes Oct 30, 2019
View reviewed changes
Copy link
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looking very good and clear! Just a few nits

@cathyxz
Copy link
Contributor

cathyxz commented Nov 1, 2019

It looks like owners bot is stuck. Could someone from @ampproject/wg-caching take a look?

@honeybadgerdontcare
Copy link
Contributor

@cathyxz I'm seeing an error via check links, not owners...

[06:36:59] Starting 'check-links'...
[06:37:00] [✖] ../amp-mega-menu/amp-mega-menu.md (400)
[06:37:00] ERROR: Possible dead link(s) found in extensions/amp-nested-menu/amp-nested-menu.md
[06:37:00] ERROR: Please update the dead link(s) in these files: extensions/amp-nested-menu/amp-nested-menu.md
[06:37:00] NOTE 1: Valid links that don't resolve on Travis can be ignored via ignorePatterns in build-system/tasks/check-links.js.
[06:37:00] NOTE 2: Links that aren't meant to resolve to a real webpage can be exempted from this check by surrounding them with backticks (`).

CrystalOnScript
CrystalOnScript approved these changes Nov 4, 2019
View reviewed changes
Copy link
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for fixing!

cathyxz
cathyxz reviewed Nov 6, 2019
View reviewed changes
extensions/amp-nested-menu/validator-amp-nested-menu.protoascii Outdated
mandatory_oneof: "['amp-nested-submenu', 'amp-nested-submenu-close', 'amp-nested-submenu-open']"
}
}
tags: { # <amp-nested-menu> <button amp-nested-submenu-open/close>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to express this rule?

within amp-nested-menu, allowed elements can contain either one but not both of these attributes: 'amp-nested-submenu-close', 'amp-nested-submenu-open'

Where allowed elements are: heading tags, buttons, or spans.

Right now, we individually list and specify them, but it would probably be easier if we had one rule and a list of tags to which they applied.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Use attr_lists, defined here. Example usage.

Code:

attr_lists {
  name: "amp-nested-menu-attrs"
  attrs: {
    name: "amp-nested-submenu"
    mandatory_oneof: "['amp-nested-submenu', 'amp-nested-submenu-close', 'amp-nested-submenu-open']"
  }
  attrs: {
    name: "amp-nested-submenu-close"
    mandatory_oneof: "['amp-nested-submenu', 'amp-nested-submenu-close', 'amp-nested-submenu-open']"
  }
  attrs: {
    name: "amp-nested-submenu-open"
    mandatory_oneof: "['amp-nested-submenu', 'amp-nested-submenu-close', 'amp-nested-submenu-open']"
  }
}

Then for each tag element specify it by name:

tags: {
  ...
  attr_lists: "amp-nested-menu-attrs"
  ...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And note my code is wrong since the ones below do not have amp-nested-submenu, only the -open and -close

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That saves writing out the individual attributes, but I guess we still have to defined the tags one by one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there are no grouping of TagSpecs which have specific requirements like this on the tag itself. There are grouping of tag names where it serves a specific purpose, like DescendantTagList.

@cathyxz
Copy link
Contributor

cathyxz commented Nov 6, 2019

@honeybadgerdontcare PTAL?

@cathyxz
Copy link
Contributor

cathyxz commented Nov 6, 2019 •
edited
Loading

Thanks a lot for the pointer! Made the change. Let me know if you have further comments. Otherwise I'd like to leave this in a mergeable state by Friday (my last day).

honeybadgerdontcare
honeybadgerdontcare requested changes Nov 6, 2019
View reviewed changes
@honeybadgerdontcare
Copy link
Contributor

Friday (my last day).

=(

honeybadgerdontcare
honeybadgerdontcare approved these changes Nov 8, 2019
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for merge.

extensions/amp-nested-menu/0.1/test/validator-amp-nested-menu-error.out
| <h4 amp-nested-submenu-open></h4>
| <div amp-nested-submenu></div>
>> ^~~~~~~~~
amp-nested-menu/0.1/test/validator-amp-nested-menu-error.html:59:12 The attribute 'amp-nested-submenu' may not appear in tag 'div'.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unfortunate error message since it really should be reporting the issue is it being a descendant of amp-accordion. We should file an issue against the validator on error selection once this is submitted so the issue can point it out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #25499. Thank you!

@cathyxz cathyxz mentioned this pull request Nov 8, 2019
Closed
@cathyxz cathyxz merged commit c4954ec into ampproject:master Nov 8, 2019
@cathyxz cathyxz mentioned this pull request Nov 8, 2019
Merged
banaag pushed a commit to banaag/amphtml that referenced this pull request Nov 12, 2019
@honeybadgerdontcare @banaag
cl/279442666 Revision bump for ampproject#25339
e28a20f
banaag pushed a commit to banaag/amphtml that referenced this pull request Nov 12, 2019
@honeybadgerdontcare @banaag
cl/279442666 Revision bump for ampproject#25339
b7c54eb
@banaag banaag mentioned this pull request Nov 12, 2019
Merged
banaag added a commit that referenced this pull request Nov 12, 2019
@banaag
Validator rollup (#25559)
b203bba 
* cl/279189628 When heights or sizes is empty, the layout should be fixed

* cl/279442666 Revision bump for #25339

* Removed unwanted files from pull request
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
@leafsy
Add validation and documentation for <amp-nested-menu> (ampproject#25339
87ef460 
)

* nested menu validation & docs

* add example file

* documentation fixes

* Simplify attribute lists

* Rename spec_names to lowercase

* Require usage since nested-menu's extension is loaded by sidebar
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
@banaag
Validator rollup (ampproject#25559)
0497899 
* cl/279189628 When heights or sizes is empty, the layout should be fixed

* cl/279442666 Revision bump for ampproject#25339

* Removed unwanted files from pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare approved these changes

@CrystalOnScript CrystalOnScript CrystalOnScript approved these changes

+1 more reviewer

@cathyxz cathyxz cathyxz approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@cathyxz cathyxz

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@leafsy @cathyxz @honeybadgerdontcare @CrystalOnScript @googlebot