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

Navigation: AMP dropdown integration and dropdown toggling #4570

Closed
1 task
tochwill opened this issue Nov 4, 2019 · 5 comments
Closed
1 task

Navigation: AMP dropdown integration and dropdown toggling #4570

tochwill opened this issue Nov 4, 2019 · 5 comments
Assignees

Comments

@tochwill
Copy link
Contributor

tochwill commented Nov 4, 2019

Is your feature request related to a problem? Please describe.
The new navigation designs contain a dropdown menu that is toggleable using a burger menu and a cross icon. The methods of achieving this are different from AMP to the canonical page.

Screenshots of the components in open/closed state can be seen here: BBC-archive/psammead#2557

Describe the solution you'd like
With React, we keep in state whether to show or hide the dropdown, and render accordingly. With AMP, we need to add a 'hidden' attribute, and use the AMP specific tap event. See Alistair's comment below for more details on this approach. To see an example, the ConsentBanner in Simorgh contains an interaction that uses the AMP tap event.

The Psammead navigation package will export AMP presentational components for the menu toggle and dropdown components. When composing the AMP navigation in Simorgh, include logic to toggle the dropdown state. The Psammead components will accept click handlers, which can be used to update the state of the navigation. This is described here: BBC-archive/psammead#2557

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Testing notes

  • Manual testing of event handling.
  • AMP specific cypress test for visibility of navigation (pair with a tester if you can).

Dev insight: Will there be any potential regression? etc

  • This feature is expected to need manual testing.

Additional context
Add any other context or screenshots about the feature request here.

@AlistairGempf
Copy link
Contributor

AlistairGempf commented Nov 5, 2019

In terms of the specifics of how the toggling works with amp, I used the on="tap:menu.toggleVisibility" attribute (where menu is the ID of the menu element) on the menu button https://github.com/bbc/psammead/blob/0a01ff1f3b472a1201ee8bd6730bab1c7dab20c6/packages/components/psammead-navigation/src/index.jsx#L401 Then we need the hidden attribute on the menu element, as well as the menu ID https://github.com/bbc/psammead/blob/0a01ff1f3b472a1201ee8bd6730bab1c7dab20c6/packages/components/psammead-navigation/src/index.jsx#L439

I think this is the simplest way of doing it, but there seem to be more complex versions that allow for animations during opening the menu doing something like this: https://amp.dev/documentation/guides-and-tutorials/develop/animations/triggering_css_animations/?format=websites

@tochwill tochwill changed the title AMP navigation dropdown and add dropdown toggling AMP navigation dropdown integration and add dropdown toggling Nov 6, 2019
@tochwill tochwill changed the title AMP navigation dropdown integration and add dropdown toggling AMP navigation dropdown integration and dropdown toggling Nov 6, 2019
@tochwill tochwill transferred this issue from BBC-archive/psammead Nov 6, 2019
@tochwill tochwill added Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. ws-frontpage-stream labels Nov 6, 2019
@tochwill tochwill added this to the Navigation (WS FP) milestone Nov 6, 2019
@tochwill tochwill changed the title AMP navigation dropdown integration and dropdown toggling Navigatin: AMP dropdown integration and dropdown toggling Nov 6, 2019
@tochwill tochwill changed the title Navigatin: AMP dropdown integration and dropdown toggling Navigation: AMP dropdown integration and dropdown toggling Nov 6, 2019
@tochwill
Copy link
Contributor Author

tochwill commented Nov 6, 2019

Blocked on BBC-archive/psammead#2557

@tochwill tochwill added the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Nov 6, 2019
@tochwill
Copy link
Contributor Author

tochwill commented Nov 6, 2019

Cheers for all your work on this @AlistairGempf - it's been really useful. I think we should go with the first option to begin with, I will create separate issues to handle animation for both canonical and AMP, and we can look into passing in a more complex AMP action at this point (covered here).

@tochwill tochwill removed the Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. label Nov 7, 2019
@tochwill
Copy link
Contributor Author

The same drowndown list components and scrollable navigation are going to be used for canonical and for AMP, with the only difference being the wrapper component and state management. I think it would be worth pairing on these tasks and doing a swarm review to try and avoid the back and forth across the board that we had with the nav components?

@AlistairGempf AlistairGempf removed the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Nov 29, 2019
@tochwill
Copy link
Contributor Author

I've merged this into one navigation integration ticket that keeps track of the nav work as a whole, as this has been merged into the feature branch but is not ready to go live until the rest of the work is complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants