-
-
Notifications
You must be signed in to change notification settings - Fork 295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core] Add mini drawer variant to DashboardLayout #4017
[core] Add mini drawer variant to DashboardLayout #4017
Conversation
Looks good! One comment on the prop name: A prop name such as In that case, the |
Looking good! I have a few remarks from checking the demo:
Not sure, wdyt? |
These suggestions sound good, thanks! The only major issue I've had trying to implement them so far is that it's difficult to make default values depend on screen size and work well with SSR. All else seems good.
Only good way I found to "fix" this, and also by comparing with a few other websites, is not to have a CSS transition here at all, because the text is too close to the icons. I think we can just remove the transition. |
Is it possible to leverage Another option could be to similarly to the responsive demo use multiple drawers and toggle them with CSS. We could use 3 drawers where desktop and tablet are almost identical except one is default open and one is default close.
You could create a second state |
My current/latest solution in this PR works as per your proposal in your first comment, except that the default drawer in tablet viewports is still the standard sidebar. Also, all the expand/collapse mechanisms are sharing one single So additionally, I guess that we want to:
The reason I haven't done this is SSR since
It should be doable with a second state like that or using CSS classes during the CSS transitions, just wasn't sure if it was worth the effort but we can do it. Also will have to change things a bit so that there's enough free space between icons and text in the sidebar. |
I think my latest commit solves the expanded/collapsed mini sidebar issue by separating two states: So other than that now just need to re-add the CSS transition and make it work, I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Looks good to me! Minimal integration load on the user, available automatically with a very simple opt-out.
Only thing that sticks out is the layout shift on the collapse animation when sections are present in the sidebar:
With sections (Janky) | Without sections (Smooth) |
---|---|
Screen.Recording.2024-09-12.at.7.16.28.PM.mov |
Screen.Recording.2024-09-12.at.7.16.01.PM.mov |
Not sure what exactly could be done about it, but I feel it would be important to address this, if not in this then in a subsequent follow-up. One idea could be to leave space even after collapse equal to the section header's height so that the icons do not appear to be shifting; analogous to what scrollbar-gutter: stable
does. Do you think this makes sense to do? Let me know.
I think collapsing them is ok. Layout shift is a problem when it happens unexpectedly (think, I want to klick a button and suddenly it drops 20 pixels). Here is just an animation from one state to another. Just it seems like right before the collapse kicks in, the subtitle drops down a few pixels. |
Perhaps we could change the hero demo to be one which doesn't include the section headers in that case - Makes sense to have the smoothest demo lead the page, I guess? |
I think the collapsing subtitles look great (except for that small glitch, but I'd consider that a bug) |
I'm not sure I like the current subtitles transition a lot either... Might prefer it without any transition actually as I don't find the "layout shift" too jarring. I'll still see if I can improve this by adjusting this a bit. |
How about this solution? Does it seem better? https://deploy-preview-4017--mui-toolpad-docs.netlify.app/toolpad/core/react-dashboard-layout/#mini-variant-sidebar It kind of preserves the usefulness of headers as much as possible and makes them behave more similar to dividers in the mini view, plus has zero layout shift. |
Seems like a good direction to go in, perhaps we could do something to indicate that the text has been truncated with a Tooltip containing the full name of the section? |
…n for items below divider
03ee6e3
to
f40a3e8
Compare
Jan preferred the previous approach. The single letters did look kind of weird in the mini sidebar, I guess it's hard to find a perfect solution. Reverted back to the old behavior but I think I found the "shifting" issue, is it fixed now? If the issue is gone I guess we can go with this for now. |
Closes #3808
Has tooltips, shows only icons for top-level items in collapsed view, can be expanded/collapsed with the top left button in any screen size.
https://deploy-preview-4017--mui-toolpad-docs.netlify.app/toolpad/core/react-dashboard-layout/#mini-variant-sidebar