✨Implement nested menu for sidebar 2.0 #24570
Conversation
There was a problem hiding this comment.
Did a quick pass, the demo looks great 😄! I'm not sure how we're handling validator rules with the 1.0 version but at some point those new attributes will need to go into validator-amp-sidebar.protoascii and we will need to make validator tests for them. It would also be great to have a few tests for this new behavior (maybe also include your demo as a manual test?). Thank you :))
There was a problem hiding this comment.
I think this looks good! I think there are only two things that we may need to think about a bit more.
- Verify that your implementation properly works with laying things out (i.e. do we need to schedule layout for its children).
- We should start compiling a list of validator rules / requirements so we can implement sufficiently robust restrictions.
Nice work!
| /** @override */ | ||
| layoutCallback() { | ||
| this.registerEventListeners_(); | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
I wonder if you have to call scheduleLayout for all its children here. A good way to test this would be to have a case where the drilldowns contain amp-img (on each submenu), and verify that the images load correctly as we switch between submenus.
There was a problem hiding this comment.
I added images on each layer and they all appear to load without issues. Is there anything else I should look out for to ensure the behavior is correct?
| this.getAmpDoc(), | ||
| 'amp-drilldown' | ||
| ); | ||
| this.drilldownMenu_ = drilldownMenu; |
There was a problem hiding this comment.
I'm assuming that the intention here is that only one drilldown menu will exist in amp-sidebar. If yes, we should also add this to validation.
There was a problem hiding this comment.
That's right; I've added some validation items in the design doc under the current approach.
There was a problem hiding this comment.
Oh actually, there is one thing. Can you add your current demos and testing code to the test/manual folder? You can create a subfolder there for amp-sidebar and name your tests amp-sidebar-with-drilldown or something like that. Since this uses amp-list, you can use endpoints from build-system/server/routes/list.js or add your own to that file.
|
@dvoytenko this is @leafsy 's work for adding nested menus to amp-sidebar that we discussed in design review last week fyi. We would appreciate your feedback if you have the time. |
| @@ -725,6 +725,13 @@ exports.extensionBundles = [ | |||
| latestVersion: '0.2', | |||
| type: TYPES.MISC, | |||
| }, | |||
| { | |||
| name: 'amp-drilldown', | |||
There was a problem hiding this comment.
I feel like historically 'drilldown menus' refer to this type of menu:
I'd recommend finding a better name for this, also we probably want to add amp-sidebar-* as a prefix for it since it doesn't seem to make sense outside of amp-sidebar if I'm right
There was a problem hiding this comment.
Since this is under experiment, and we have an ongoing discussion about naming, let's not block on this as we can find/replace the name once we reach an agreement.
Demo: https://amp-sidebar-nested.firebaseapp.com/
Implemented according to the API found here
Add nested menu functionality to the next version of
amp-sidebar, currently under experiment.UPDATE: per recent discussions, the submenu logic has been extracted into a separate component (tentatively named
amp-drilldown), which should only reside inside sidebar (this will be enforced via validation later on)./cc @cathyxz @sparhami @wassgha for review