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

Integrate canonical navigation animation #5057

Merged
merged 9 commits into from
Jan 10, 2020

Conversation

OlgaLyubin
Copy link
Contributor

Resolves #NUMBER

Overall change:
Pull in new version of navigation from Psammead and add canonical animation to the new navigation.

Code changes:

  • Update version of navigation
  • Separate dropdown into amp and canonical
  • Update tests

  • I have assigned myself to this PR and the corresponding issues
  • I have added labels to this PR for the relevant pod(s) affected by these changes
  • I have assigned this PR to the Simorgh project

Testing:

  • Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
  • If necessary, I have run the local E2E non-smoke tests relevant to my changes (CYPRESS_APP_ENV=local CYPRESS_SMOKE=false npm run test:e2e:interactive)
  • This PR requires manual testing

@OlgaLyubin
Copy link
Contributor Author

Jenkins is failing on rendering a Brand with a Skip to content link. However, that issue is being fixed in #4948 .

@AlistairGempf AlistairGempf mentioned this pull request Jan 8, 2020
4 tasks
Copy link
Contributor

@AlistairGempf AlistairGempf left a comment

Choose a reason for hiding this comment

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

Looks good

@@ -30,6 +30,14 @@ describe('defaultPageWrapper', () => {
},
};

window.matchMedia = jest.fn().mockImplementation(() => {
Copy link
Contributor

@DenisHdz DenisHdz Jan 8, 2020

Choose a reason for hiding this comment

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

I think we can skip this since we won't need the hook once we pull in the changes in this PR BBC-archive/psammead#2875

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, removed it

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.

LGTM 👍

@PriyaKR
Copy link
Contributor

PriyaKR commented Jan 9, 2020

Testing:

Have verified the changes that have gone in this integration.Looks good except the navigation dropdown menu doesn't animate on ios device(tested on ios 13,12).

@OlgaLyubin
Copy link
Contributor Author

Testing:

Have verified the changes that have gone in this integration.Looks good except the navigation dropdown menu doesn't animate on ios device(tested on ios 13,12).

Thank you Priya! Looking into it now.

@PriyaKR
Copy link
Contributor

PriyaKR commented Jan 10, 2020

The ios issue would be fixed in a separate PR and its good to merge.

@OlgaLyubin
Copy link
Contributor Author

Ticket for the IOS issue that will be dealt with separately: BBC-archive/psammead#2898

@OlgaLyubin OlgaLyubin merged commit 884fb79 into integrate-new-navigation Jan 10, 2020
@OlgaLyubin OlgaLyubin deleted the nav-animation branch January 10, 2020 11:15
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 this pull request may close these issues.

5 participants