Optimize drawer width to reduce wasted space in reports and settings #1295
Conversation
|
I don't think it looks good visually. |
src/common/components/PageLayout.jsx
Outdated
| @@ -27,7 +27,8 @@ const useStyles = makeStyles((theme) => ({ | |||
| flexDirection: 'column', | |||
| }, | |||
| desktopDrawer: { | |||
| width: theme.dimensions.drawerWidthDesktop, | |||
| maxWidth: theme.dimensions.drawerWidthDesktop, | |||
| position: 'relative !important', | |||
There was a problem hiding this comment.
otherwise the right content will use the full width and the Drawer will float on top of it.
There was a problem hiding this comment.
Emulator page also puts "position: relative".
For the !important, don't know why but it doesn't work without it.
|
Another option would be to add a collapse button to show only the icons. It would help a lot, especially when you start adding columns to reports. |
|
Report columns are usually configurable. |
|
Yes I known, what I mean is that we quickly get an horizontal scroll bar when we start adding columns to a report. |
|
Would be great if we can make them manually adjustable, but current size by default. |
|
ok, and what about a collapse button on the side bar to gain space? |
|
I guess it works if it looks good. |
|
ok, I'll work on that |
jcardus
force-pushed
the
drawer-width
branch
from
2a54337
to
62f6da1
Compare
Screen.Recording.2024-11-14.at.12.16.06.mov |
| @@ -22,7 +22,7 @@ const MenuItem = ({ | |||
| }) => ( | |||
| <ListItemButton key={link} component={Link} to={link} selected={selected}> | |||
| <ListItemIcon>{icon}</ListItemIcon> | |||
| <ListItemText primary={title} /> | |||
| <ListItemText primary={title} sx={{ whiteSpace: 'nowrap' }} /> | |||
There was a problem hiding this comment.
What is this for? Also probably it should be set as a class.
There was a problem hiding this comment.
When we change the width of the sidebar, multi word items break the line and then the height of the item grows
src/common/components/PageLayout.jsx
Outdated
| const theme = useTheme(); | ||
| const navigate = useNavigate(); | ||
|
|
||
| const desktop = useMediaQuery(theme.breakpoints.up('md')); | ||
|
|
||
| const [openDrawer, setOpenDrawer] = useState(false); | ||
|
|
||
| const toggleDrawer = () => { |
| <PageTitle breadcrumbs={breadcrumbs} /> | ||
| </> | ||
| )} | ||
| <IconButton color="inherit" edge="start" sx={{ ml: miniVariant ? -2 : 'auto' }} onClick={toggleDrawer}> |
There was a problem hiding this comment.
I think this needs to be reformatted.
There was a problem hiding this comment.
I think we should probably apply classes here as well for readability.
There was a problem hiding this comment.
I followed the same pattern as the upper IconButton
src/common/theme/dimensions.js
Outdated
| @@ -2,6 +2,7 @@ export default { | |||
| sidebarWidth: '28%', | |||
| sidebarWidthTablet: '52px', | |||
| drawerWidthDesktop: '360px', | |||
| miniDrawerWidthDesktop: '56px', | |||
There was a problem hiding this comment.
Is there some standard size we can use from MUI?
src/common/components/PageLayout.jsx
Outdated
| @@ -27,7 +29,11 @@ const useStyles = makeStyles((theme) => ({ | |||
| flexDirection: 'column', | |||
| }, | |||
| desktopDrawer: { | |||
| width: theme.dimensions.drawerWidthDesktop, | |||
| width: (props) => (props.miniVariant ? `calc(${theme.spacing(7)} + 1px)` : theme.dimensions.drawerWidthDesktop), | |||
There was a problem hiding this comment.
calc(${theme.spacing(7)} + 1px) doesn't seem right. I know there is a version of a collapsed sidebar. Can we somehow use the size from there?
There was a problem hiding this comment.
Or maybe even use it as is instead of implementing our own thing.
https://mui.com/material-ui/react-drawer/#mini-variant-drawer
There was a problem hiding this comment.
That's exactly where that was taken from
There was a problem hiding this comment.
But can we re-use the component instead of just copy-pasting the style?
There was a problem hiding this comment.
It's not a component, it's an example how to do the mini variant.
There was a problem hiding this comment.
But I used the size they use for small screens (spacing(7)) in the example because using the other (spacing(8)) our icons wouldn't be centered. To use the mui example we should use spacing(8). Here's the difference:
using 7:
using 8:
| whiteSpace: 'nowrap', | ||
| }, | ||
| }); | ||
|
|
||
| const MenuItem = ({ |
There was a problem hiding this comment.
Maybe we can extract it as a common component to avoid duplication?
src/common/components/MenuItem.jsx
Outdated
| const classes = makeStyles({ | ||
| menuItemText: { | ||
| whiteSpace: 'nowrap', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Can you please move this outside for consistency. Similar to how we have it in other places.
before:
after:
before:
after:
before:
after:
