Improve Nav Bar & use Wasp Link components #311
Conversation
|
@vincanger maybe best to have @infomiho take a look at this since he is the author of Wasp Link stuff? |
| import { useMemo } from 'react'; | ||
| import { useLocation } from 'react-router-dom'; | ||
|
|
||
| export const useIsLandingPage = () => { |
There was a problem hiding this comment.
Nice job on creating custom hooks :)
template/app/src/client/App.tsx
Outdated
| @@ -49,7 +53,9 @@ export default function App() { | |||
| <Outlet /> | |||
| ) : ( | |||
| <> | |||
| {shouldDisplayAppNavBar && <AppNavBar />} | |||
| {shouldDisplayAppNavBar && ( | |||
| <NavBar navigation={isLandingPage ? landingPageNavigationItems : appNavigationItems} /> | |||
There was a problem hiding this comment.
One thing you could do here is extract the ternary expresison:
const navigationItems = isLandingPage ? landingPageNavigationItems : appNavigationItems;
...
<NavBar navigation={navigationItems} />That being said, it looks to me that the navigation prop should be named navigationItems since we are calling all the vars that way xNavigationItems.
|
|
||
| const NavLogo = () => <img className='h-8 w-8' src={logo} alt='Your SaaS App' />; | ||
|
|
||
| export default function AppNavBar() { | ||
| export default function AppNavBar({ navigation }: { navigation: NavigationItem[] }) { |
There was a problem hiding this comment.
I'd name the navigation prop as navigationItems so it matches the way we use it. navigation feels generic as opposed to navigationItems.
| @@ -124,3 +112,28 @@ export default function AppNavBar() { | |||
| </header> | |||
| ); | |||
| } | |||
|
|
|||
| function renderNavigationItems( | |||
| navigation: NavigationItem[], | |||
There was a problem hiding this comment.
| navigation: NavigationItem[], | |
| navigationItems: NavigationItem[], |
|
|
||
| return navigation.map((item) => { | ||
| return ( | ||
| <Link |
There was a problem hiding this comment.
I'd maybe import both Link from wasp/client/router and react-router-dom and use them in different places.
- I'd use the
import { Link as ReactRouterLink } from 'react-router-dom';and<ReactRouterLink>component in places where we are having dynamictoprop (in this fn) - I'd use the Wasp's
Linkelsewhere where we use thetoprop explicitly - we get as much out of the type safe component as possible.
template/app/src/client/App.tsx
Outdated
|
|
||
| const shouldDisplayAppNavBar = useMemo(() => { | ||
| return location.pathname !== '/' && location.pathname !== '/login' && location.pathname !== '/signup'; | ||
| return location.pathname !== '/login' && location.pathname !== '/signup'; |
There was a problem hiding this comment.
To avoid hard-coding values, you could use routes.LoginRoute.build() and routes.SignupRoute.build() to get the routes :)
TODO
item.totypeapp_diffDescription
Fixes #264 & #181 as well as merges the landing page's navbar and the
AppNavBarinto oneContributor Checklist