Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Create dropdown navigation #2613

Merged
merged 121 commits into from
Nov 29, 2019
Merged

Create dropdown navigation #2613

merged 121 commits into from
Nov 29, 2019

Conversation

OlgaLyubin
Copy link
Contributor

@OlgaLyubin OlgaLyubin commented Nov 12, 2019

Resolves #2557

Overall change:
Implemented new navigation designs that contain a burger menu button which opens a dropdown of navigation items when selected. For now these are just presentational components that will be exported to Simorgh (where the logic for interaction will be implemented).

Toggling for AmpMenuButton now works in storybook (keep in mind that you might need to refresh the page because of the AMP related issue that exists in storybook for now). AmpMenuButton uses expandedHandler to update the expanded value of a state and depending on the value changes the icon to be rendered (hamburger or cross).

Code changes:

  • Imported Navigation icons from psammead-assets
  • Made hamburger and cross buttons
  • Added a dropdown navigation for amp and canonical versions
  • Added snapshot testing for dropdown navigation

  • I have assigned myself to this PR and the corresponding issues
  • Automated jest tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@OlgaLyubin OlgaLyubin added ws-home Tasks for the WS Home Team ws-fp-phase3 labels Nov 12, 2019
@OlgaLyubin OlgaLyubin added this to the Navigation (WS FP) milestone Nov 12, 2019
@OlgaLyubin OlgaLyubin self-assigned this Nov 12, 2019
@DenisHdz DenisHdz requested review from Bopchy and AlistairGempf and removed request for dr3 November 28, 2019 11:46
const storiesWithoutBrand = storiesOf(
'Components|Navigation/without brand',
module,
)
.addDecorator(withKnobs)
.addDecorator(ampDecorator)
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything looks really good to me 👍 . Just wondering if adding an ampDecorator to all the storiesWithoutBrand was on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was on purpose in order to make the AMP menu button work. If you click on the AMP button now, the menu icon will change

Copy link
Contributor

@Bopchy Bopchy left a comment

Choose a reason for hiding this comment

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

Other than the comment on the ampDecorator, everything looks good 👍

Copy link
Contributor

@DenisHdz DenisHdz left a comment

Choose a reason for hiding this comment

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

Good job on the AMP side 👍

@PriyaKR
Copy link
Contributor

PriyaKR commented Nov 29, 2019

@OlgaLyubin is the hamburger menu svg supposed to be in red colour?I can see that its in black colour.

@tochwill
Copy link
Contributor

@OlgaLyubin is the hamburger menu svg supposed to be in red colour?I can see that its in black colour.

Hey! It's transparent so it's the same colour as the menu behind it

@PriyaKR
Copy link
Contributor

PriyaKR commented Nov 29, 2019

Looks good to me except one small font size with the dropdown menu ie) at 360-599px expected font size is 15px actual its 16px.If UX is fine with it then this PR can be merged.

@PriyaKR
Copy link
Contributor

PriyaKR commented Nov 29, 2019

UX is fine with the above small issue.

@PriyaKR PriyaKR merged commit f9a8d95 into latest Nov 29, 2019
@PriyaKR PriyaKR deleted the burger-dropdown-nav branch November 29, 2019 13:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ws-home Tasks for the WS Home Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation: Create dropdown and toggle components
10 participants