Completely hide sidebar in mobile mode #7481
Conversation
Manage this branch in SquashTest this branch here: https://kaedrohosidebar-mobile-enhance-0cyl7.squash.io |
kaedroho
force-pushed
the
sidebar-mobile-enhancements-2
branch
from
201e0aa
to
d62915c
Compare
There was a problem hiding this comment.
Some thoughts:
- Is it necessary for
opento be separate fromcollapsed? A few scenarios.- I'm working solely on mobile, whether
open==collapseddoesn't affect me - I'm working solely at desktop width, similarly unaffected
- I'm resizing my screen. Now, if I have the opened/wide menu at mobile width and I expand again, the menu collapses, which feels weird. I feel like just using the
isMobileandcollapsedwe could do away withopenalltogether, but I may be missing some reason it's essential.
- I'm working solely on mobile, whether
Generally this is looking great though, and worked very well when I was testing
| @@ -180,8 +184,9 @@ export const Sidebar: React.FunctionComponent<SidebarProps> = ( | |||
| <aside | |||
There was a problem hiding this comment.
Question: would <nav> be more appropriate than <aside> here?
There was a problem hiding this comment.
<aside> is what we use for the existing sidebar. But perhaps we should be using <nav> for the main menu portion of it?
| // 'open' indicates whether the sidebar is visible or not. | ||
| // this is meant to be used to show/hide the menu on small screens. | ||
| // it only has effect on small screens. | ||
| const [open, setOpen] = React.useState(false); |
There was a problem hiding this comment.
Could we rename open to something more intuitive here? I feel like the set of variables we've got to describe the menu state is getting slightly confusing with this addition (since I'd probably expect open to be eg the opposite of collapsed, but they're really describing different things). Maybe visible?
There was a problem hiding this comment.
Oh, good point! Yep I've renamed this to visibleOnMobile
kaedroho
force-pushed
the
sidebar-mobile-enhancements-2
branch
from
d62915c
to
8b3d3ad
Compare
kaedroho
force-pushed
the
sidebar-mobile-enhancements-2
branch
from
8b3d3ad
to
59c33de
Compare
* Rename 'open' to 'visibleOnMobile' * Don't initialise 'collapsed' state based on screen size (old code) * Remove main.sidebar--open (not used)
I think so, since |
This PR completely hides the sidebar on mobile, and adds a hamburger icon to allow toggling it.
Extracted from #7362
Based on #7469