DUOS-810 [risk=no] Swapped out ResponsiveMenu for conditional rendering of navbar #885
Conversation
… more with styling
…d styling), adjustment to duos-navbar z-index value
…keleton links, updated styling
src/components/DuosHeader.js
Outdated
| const dropdownLinks = { | ||
| statistics: { | ||
| 'Votes Statistics': { | ||
| isRendered: !(isDataOwner || isResearcher) || isAdmin, |
There was a problem hiding this comment.
I would prefer to have explicit roles defined in the affirmative here instead of the negative. For example, if we add a new role, say, "isSigningOfficial" and we don't explicitly negate that role here, then that role will gain access to this link. Same feedback for reviewed cases.
There was a problem hiding this comment.
This was the pre-existing condition on the navbar, but it's not obvious who's limited to seeing it (why Admin but not DataOwner or Researcher? Can a Researcher be a DAC member? What happens then?, etc.)
There was a problem hiding this comment.
Also I need to remove the conditional on Votes Statistics, there's no conditional on that link right now.
There was a problem hiding this comment.
Statistics are admin only right now, which is why we should clean this up. At some point, DAC Chairs will have a version of this page to look at, but we don't have it yet.
src/components/DuosHeader.js
Outdated
| h(DropdownComponent, {isRendered: isAdmin, label: 'Statistics', goToLink: this.goToLink, onMouseEnter: applyPointer, dropdownLinks: dropdownLinks.statistics, classes}), | ||
| h(BasicListItem, {isRendered: isLogged, applyPointer, targetLink: '/dataset_catalog', label: 'Dataset Catalog', goToLink: this.goToLink}), | ||
| h(BasicListItem, {isRendered: !isLogged, applyPointer, targetLink: '/home_about', label: 'About', goToLink: this.goToLink}), | ||
| h(BasicListItem, {isRendered: !isLogged, applyPointer, targetLink: '/FAQs', label: 'FAQs', goToLink: this.goToLink}), |
There was a problem hiding this comment.
We show FAQs now in both logged in and non-logged in state. We should keep that visible in both versions
There was a problem hiding this comment.
ul({ isRendered: !isLogged, className: 'navbar-public' }, [
li({}, [
h(Link, { id: 'link_about', className: 'navbar-duos-link', to: '/home_about' }, [
div({ className: 'navbar-duos-icon-about', style: navbarDuosIcon }),
span({ style: navbarDuosText }, ['About'])
])
]),
li({}, [
h(Link, { id: 'link_help', className: 'navbar-duos-link', to: '/FAQs' }, [
div({ className: 'navbar-duos-icon-help', style: navbarDuosIcon }),
span({ style: navbarDuosText }, ['FAQs'])
])
]),
contactUsButton, supportrequestModal
])
])
This is still present, which means it's a conditional duplicate, so I might as well remove it.
There was a problem hiding this comment.
Nevermind, didn't realize the current navbar maintains ul tags for logged in/logged out states
Co-authored-by: Greg Rushton <rushtong@users.noreply.github.com>
… conditional for Votes Statistics
…le conditional for statistics
…, rearranged FAQ position in links
…e tests for about page
| @@ -14,7 +116,8 @@ class DuosHeader extends Component { | |||
| this.state = { | |||
| showSupportRequestModal: false, | |||
| hover: false, | |||
| dacChairPath: '/chair_console' | |||
| dacChairPath: '/chair_console', | |||
There was a problem hiding this comment.
Not for this PR, but https://broadworkbench.atlassian.net/browse/DUOS-1045 should address the bug that we aren't dynamically checking for the correct console path here.
| img({ | ||
| style: duosLogoImage, src: '/images/duos_logo.svg', | ||
| alt: 'DUOS Logo', | ||
| onClick: (e) => this.goToLink('/home') | ||
| }), |
There was a problem hiding this comment.
The only thing we lost here is the cursor hover style over the icon. In expanded mode, it works, but here, it's a normal arrow icon. I think that has to do with the css class name in the expanded version.
|
The failing npm audit check is a false positive: facebook/create-react-app#10578 |
Addresses DUOS-810 since the navbar's dependence on react-responsive menu is holding back the upgrade to React 17.
PR aims to replace the ResponsiveMenu with conditional rendering in line with Material-UI style components. This ultimately results in the conditional rendering of a desktop navbar and a mobile/condensed navbar depending on the client's screen width. The PR sets the breakpoint for the switch at the upper range for "medium" sized widths (as described by material-ui docs), though that can be adjusted based on feedback. PR also poses a question for displaying navbar links: Should dropdowns be implemented or should they be displayed as separate links? (also to be adjusted based on feedback).
NOTE: Work can be done to avoid the dual navbar implementation, but that falls outside of the scope of this PR. The focus right now is to pave the way for a React 17 upgrade while providing a functional navbar for smaller devices. Creating a singular, flexible navbar would be close to a rewrite since the current navbar is heavily styled by element (not class) specific css.
Have you read Terra's Contributing Guide lately? If not, do that first.
I, the developer opening this PR, do solemnly pinky swear that:
In all cases: