feat: Add mobile navbar #3186
feat: Add mobile navbar #3186
Conversation
| @@ -19,41 +23,66 @@ export const Language = { | |||
| users: "Users", | |||
| } | |||
|
|
|||
| export const NavbarView: React.FC<NavbarViewProps> = ({ user, onSignOut }) => { | |||
| const NavItems: React.FC<{ className?: string; linkClassName?: string }> = ({ className }) => { | |||
There was a problem hiding this comment.
You can just import FC as a standalone if you wish
| const styles = useStyles() | ||
| const location = useLocation() | ||
|
|
||
| return ( | ||
| <List className={combineClasses([styles.navItems, className])}> |
There was a problem hiding this comment.
What's the benefit of using combineClasses instead of template literals?
There was a problem hiding this comment.
It handles, null, undefined and conditional styles like { isActive: styles.active }.
|
I think we can add a storybook for this at a smaller screensize: https://storybook.js.org/docs/react/essentials/viewport |
|
|
||
| export const NavbarView: React.FC<NavbarViewProps> = ({ user, onSignOut }) => { | ||
| const styles = useStyles() | ||
| const [isDrawerOpen, setIsDrawerOpen] = useState(false) |
There was a problem hiding this comment.
nit Maybe worth a discussion in FE Variety, but I think by convention the setter function need not have the verb like is or has. For example, I would use setDrawerOpen here.
There was a problem hiding this comment.
I think this would be nice!
| [theme.breakpoints.up("md")]: { | ||
| bottom: 0, | ||
| left: theme.spacing(3), | ||
| width: `calc(100% - 2 * ${theme.spacing(3)}px)`, |
There was a problem hiding this comment.
nit A comment describing the calculation here would be nice.
There was a problem hiding this comment.
I put the commit on auto merge but I will address this when I touch it again.
Related to: #3185