search

mui / toolpad

Toolpad: Full stack components and low-code builder for dashboards and internal apps.
https://mui.com/toolpad/
MIT License
1.27k stars 282 forks source link

[core] Add mini drawer variant to DashboardLayout #4017

Closed apedroferreira closed 1 month ago

apedroferreira commented 2 months ago

Closes https://github.com/mui/mui-toolpad/issues/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

bharatkashyap commented 2 months ago

Looks good! One comment on the prop name: A prop name such as sidebarCollapsible with a boolean type would probably be more descriptive to me, since this prop adds collapsibility to the existing sidebar, versus being a "variant" which would indicate to me a completely different type of sidebar layout such as tabs, along with more features exclusive to this variant.

In that case, the boolean variables used to control this feature inside the component code could also become more understandable at first glance: sidebarCollapsible and isSidebarCollapsed versus isMiniVariant and isExpanded.

Janpot commented 2 months ago

Looking good! I have a few remarks from checking the demo:

Not sure, wdyt?

apedroferreira commented 2 months ago

Looking good! I have a few remarks from checking the demo:

  • Personally, not super convinced by the variant prop. What if we just did it without this prop and have the following default behavior:

    • drawer open on desktop (but with toggle to collapse to mini drawer)
    • mini drawer on tablet (but with toggle to collapse open as an overly, like mobile)
    • collapsed with toggle to open as overlay on mobile

  • We should think about the behavior for when there are no icons

  • The mini variant feels more like an addition. If we make it configurable, perhaps it makes sense to just have a disableMiniDrawer or something for when users don't have icons and don't want the default behavior?

Not sure, wdyt?

Looks good! One comment on the prop name: A prop name such as sidebarCollapsible with a boolean type would probably be more descriptive to me, since this prop adds collapsibility to the existing sidebar, versus being a "variant" which would indicate to me a completely different type of sidebar layout such as tabs, along with more features exclusive to this variant.

In that case, the boolean variables used to control this feature inside the component code could also become more understandable at first glance: sidebarCollapsible and isSidebarCollapsed versus isMiniVariant and isExpanded.

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. So I think I will generally follow both suggestions but create an optional enableMiniDrawer prop or similar that defaults to being disabled, as in big screens we don't want the default to be mini, but in mobile we don't want to start with an expanded sidebar.

  • When I close the drawer, the text disappears before it's fully collapsed. It would look better if the text is visible while collapsing

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.

Janpot commented 2 months ago

but create an optional enableMiniDrawer prop or similar that defaults to being disabled, as in big screens we don't want the default to be mini, but in mobile we don't want to start with an expanded sidebar.

Is it possible to leverage useMediaQuery to make this demo responsive? (Try to enlarge the width to desktop size in codesandbox). We may have to disable it during SSR though.

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.

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.

You could create a second state fullyClosed as per this codesandbox that is only true when fully closed. Then toggle the content based on that state.

apedroferreira commented 2 months ago

Is it possible to leverage useMediaQuery to make this demo responsive? (Try to enlarge the width to desktop size in codesandbox). We may have to disable it during SSR though.

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.

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. If enableMiniSidebar is false as per default, it's not possible to expand/collapse to a mini sidebar. If enableMiniSidebar is set to true, the mini sidebar becomes the default for tablet and desktop views but can be expanded.

Also, all the expand/collapse mechanisms are sharing one single isNavigationExpanded state for all viewports right now.

So additionally, I guess that we want to:

The reason I haven't done this is SSR since isNavigationExpanded is a single state that right now would have to be true in desktop and false in tablet, but I can't determine that on the server-side render. It might be solvable with multiple states. Trying to read the viewport size during SSR seemed like overkill just for this. I can give it another try.

You could create a second state fullyClosed as per this codesandbox that is only true when fully closed. Then toggle the content based on that state.

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.

apedroferreira commented 2 months ago

I think my latest commit solves the expanded/collapsed mini sidebar issue by separating two states: isDesktopNavigationExpanded and isMobileNavigationExpanded, seems to work well.

So other than that now just need to re-add the CSS transition and make it work, I think.

Janpot commented 1 month ago

Only thing that sticks out is the layout shift on the collapse animation when sections are present in the sidebar

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.

bharatkashyap commented 1 month ago

Only thing that sticks out is the layout shift on the collapse animation when sections are present in the sidebar

I think collapsing them is ok. Layout shift is a problem when it happens unexpectedly (think, I want to click 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?

Janpot commented 1 month ago

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)

apedroferreira commented 1 month ago

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.

apedroferreira commented 1 month ago

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.

bharatkashyap commented 1 month ago

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?

apedroferreira commented 1 month ago

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? I guess it was happening to headers under dividers only, I had missed it. https://deploy-preview-4017--mui-toolpad-docs.netlify.app/toolpad/core/react-dashboard-layout/#mini-variant-sidebar

If the issue is gone I guess we can go with this for now.