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 Integration #4571

Closed
33 of 34 tasks
tochwill opened this issue Nov 6, 2019 · 2 comments · Fixed by #5234
Closed
33 of 34 tasks

Navigation Integration #4571

tochwill opened this issue Nov 6, 2019 · 2 comments · Fixed by #5234
Assignees

Comments

@tochwill
Copy link
Contributor

tochwill commented Nov 6, 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
The Canonical navigation should use state to keep track of whether or not the dropdown should be displayed, and render the component accordingly.

The Psammead navigation package will export canonical presentational components for the menu toggle and dropdown components. When composing the Canonical 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

With AMP, we need to add a 'hidden' attribute, and use the AMP specific tap event. 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.

Subtasks

Subtasks:

  • Create lists for dropdown and scrollable in index.js
  • Pass those lists as props
  • Fix tabbing issue - fixing when isScrollable is set, the difference between server and client render.
  • Include translations for menu button announced text.
  • Add function to handle onOpen and onClose.
  • How do we use AMP state initial JSON when simorgh converts quotes to escaped entities? This means that navigation toggling isn't working for AMP yet.
  • Figure out positioning of button and navigation.
  • Add props validation in containers
  • Fix button spacing with different line heights
  • Fix Simorgh tests
  • Change nav bar color on menu opening
    • Canonical
    • AMP
  • AMP state management
  • Also pull in the new version from psammead which has “clear:both”
  • High contrast fix in firefox
  • E2E test: AMP state toggling
  • Ensure front page E2E nav tests continue to work correctly
  • Integrate script link (and skip to content link) @Bopchy (in CR)
  • Add onClick handler to scriptLink that sets cookies for chosen variant @Bopchy (in CR)
  • Last ‘rtl’ link not displaying properly
  • Make onClose and onOpen prop into one
  • Display nav bar when window size is increased with dropdown open
    • Amp
    • Canonical
  • Spread additional Navigation props - Add id, ampOpenClass props to Navigation BBC-archive/psammead#2785 (comment)
  • Call Navigation container NavigationContainer for consistency
  • Add comment to explain what hidden={isAmp} is doing
  • Look at refactoring creating the lists into a shared function for both navigations.
  • Refactor the container to choose the component required (Amp or Canonical) rather than use a ternary.
  • Move navigationSection in the config object into the translations object, and just call it 'section'.
  • Animations: Navigation: Dropdown Entry and Exit Animations #4579 @OlgaLyubin and @AlistairGempf
  • Remove aria-hidden and tabindex -1 from scrollable nav

Testing notes
Cypress tests will need to be updated to reflect the changes in navigation. We currently check for visible nav, with a skiplink to h1, and check that no links 404. We will now have the scrollable navigation and dropdown navigation to consider - and the dropdown links will not be visible until the burger menu has been selected.

We will need to manually test state management, and write a new E2E test to cover AMP state management and click handling works correctly.

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.

@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
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 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 18, 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 tochwill changed the title Navigation: Canonical dropdown integration and dropdown toggling Navigation Integration Dec 19, 2019
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

Successfully merging a pull request may close this issue.

3 participants