Add navigation bar and logout #109
Conversation
|
|
||
| import PrivateRoute from './components/PrivateRoute'; | ||
|
|
||
| const privateRoutes = ( | ||
| <PrivateRoute> | ||
| <Routes> | ||
| <Route path='/welcome' element={<Outlet />} /> |
There was a problem hiding this comment.
Yeah, I found that this was the way to suppress warnings about "are you sure you want to render nothing".
src/app/src/components/NavBar.js
Outdated
| import { useLocation, useNavigate } from 'react-router-dom'; | ||
| import { ArrowLeftIcon, CogIcon, LogoutIcon } from '@heroicons/react/outline'; | ||
| import apiClient from '../api/client'; | ||
| import { API_URLS, NAVBAR_HEIGHT, NAVBAR_VARIANTS } from '../constants'; |
There was a problem hiding this comment.
Does having NAVBAR_VARIANTS in constants.js as opposed to here offer any benefit?
There was a problem hiding this comment.
Probably nah!
There was a problem hiding this comment.
Moved out in 31559a2
src/app/src/components/NavBar.js
Outdated
| <IconButton | ||
| icon={<Icon as={CogIcon} />} | ||
| aria-label='Settings' | ||
| onClick={() => { | ||
| navigate('/settings'); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
From the wireframes, it seems like the settings icon should not be visible on the draw page.
There was a problem hiding this comment.
Ah, right, thanks. 9309e80
There was a problem hiding this comment.
I have a feeling this might be added back in #94, but we can future that.
Good spot. I can reproduce on develop by clicking back button from draw page back to welcome (after having uploaded a ref image). I'll open an issue. Sourced it to 1d43bbe. |
src/app/src/store/authSlice.js
Outdated
| setSelectedUtility: (state, { payload: utility }) => { | ||
| state.selectedUtility = utility; |
There was a problem hiding this comment.
Consider making this more consistent:
| setSelectedUtility: (state, { payload: utility }) => { | |
| state.selectedUtility = utility; | |
| setSelectedUtility: (state, { payload: selectedUtility }) => { | |
| state.selectedUtility = selectedUtility; |
src/app/src/components/NavBar.js
Outdated
| {maybeSettingsButton} | ||
| {exitButton} |
There was a problem hiding this comment.
Can these (and utilitySelectOrLabel) be decomposed into their own components in this file?
It would make the NavBar component easier to read. We will have to call some hooks twice (useDispatch, useNavigation), but hopefully React optimizes that under the hood enough to not matter.
I'm thinking something like this:
<Flex align='center' gap={4}>
<Spacer />
<SettingsButton variant={variant} />
<ExitButton variant={variant} />
</Flex>There was a problem hiding this comment.
I like that a lot. What do you think about including dispatch as a prop? d21cac6
There was a problem hiding this comment.
My preference is for the hook because it reduces the dependecy between components.
src/app/src/components/NavBar.js
Outdated
| ); | ||
| } | ||
|
|
||
| export default function NavBar() { |
There was a problem hiding this comment.
One final comment: Can we move the NavBar component to be the first in this file? So we don't have to scroll all the way down to see the main component. Can scroll down optionally to see detailed implementations of sub-components.
jacobtylerwalls
force-pushed
the
jw/logout-bar
branch
from
8f8a623
to
31d0cff
Compare
|
Thanks for the reviews! |
Overview
Implement a navigation bar with logout. Placeholders for selected and available utilities for now.
Closes #85
Demo
Submissions activity
Draw activity (update: settings icon removed)
Notes
Minor nit: the updated wireframe says "Save & Back" but the prior draw tool had "Save and back". I opted for the existing wording "Save and back" to match "Review and submit".
Settings button is just a placeholder.
Testing Instructions
Checklist
fixup!commits have been squashedCHANGELOG.mdupdated with summary of features or fixes, following Keep a Changelog guidelinesREADME.mdupdated if necessary to reflect the changes