Skip to content
This repository was archived by the owner on Mar 6, 2025. It is now read-only.

STAR-620 add sub navigation#207

Merged
woosie10 merged 25 commits intomasterfrom
sub-navigation
Jun 5, 2019
Merged

STAR-620 add sub navigation#207
woosie10 merged 25 commits intomasterfrom
sub-navigation

Conversation

@woosie10
Copy link
Copy Markdown
Contributor

@woosie10 woosie10 commented May 28, 2019

This PR adds sub navigation as well as refactoring the existing navigation into smaller components.

The design is based on the following design, but kept more inline with our current style.
https://app.zeplin.io/project/5b18f693e66f3e4636a2a54a/screen/5cdea856407b951d80b99613

@woosie10 woosie10 changed the title [WIP] STAR-620 add sub navigation STAR-620 add sub navigation Jun 3, 2019
@ImreBognarUltimaker ImreBognarUltimaker changed the title STAR-620 add sub navigation [WIP]STAR-620 add sub navigation Jun 4, 2019
@ImreBognarUltimaker ImreBognarUltimaker changed the title [WIP]STAR-620 add sub navigation STAR-620 add sub navigation Jun 4, 2019
>
<nav
className="navigation__container"
role="navigation"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is not necessary to add navigation role to a nav element.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Navigation_Role

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

private _onCloseMobileMenuHandler(): void {
const { onCloseMobileMenuHandler } = this.props;

if (onCloseMobileMenuHandler) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The else of this if is not covered by the tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added extra test

@woosie10 woosie10 merged commit e43e6cf into master Jun 5, 2019
@woosie10 woosie10 deleted the sub-navigation branch June 5, 2019 13:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants