Skip to content
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

Merged
merged 33 commits into from
Sep 13, 2024

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Aug 29, 2024

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.

  • Add documentation

https://deploy-preview-4017--mui-toolpad-docs.netlify.app/toolpad/core/react-dashboard-layout/#mini-variant-sidebar

@apedroferreira apedroferreira added the feature: Components Button, input, table, etc. label Aug 29, 2024
@apedroferreira apedroferreira self-assigned this Aug 29, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 29, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 29, 2024
@apedroferreira apedroferreira marked this pull request as ready for review August 29, 2024 16:06
@apedroferreira apedroferreira requested a review from a team August 29, 2024 16:09
@bharatkashyap
Copy link
Member

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
Copy link
Member

Janpot commented Aug 30, 2024

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
  • When I close the drawer, the text disappears before it's fully collapsed. It would look better if the text is visible while collapsing
  • 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?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 2, 2024
@apedroferreira
Copy link
Member Author

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.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 3, 2024
@Janpot
Copy link
Member

Janpot commented Sep 3, 2024

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
Copy link
Member Author

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:

  • Have mini sidebar expand/collapse behavior enabled by default instead of disabled
  • Default to mini sidebar when tablet viewports first load

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
Copy link
Member Author

apedroferreira commented Sep 3, 2024

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.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 6, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 6, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 11, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 12, 2024
Copy link
Member

@bharatkashyap bharatkashyap left a 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.

@Janpot
Copy link
Member

Janpot commented Sep 12, 2024

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
Copy link
Member

bharatkashyap commented Sep 12, 2024

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
Copy link
Member

Janpot commented Sep 12, 2024

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
Copy link
Member Author

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
Copy link
Member Author

apedroferreira commented Sep 12, 2024

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
Copy link
Member

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
Copy link
Member Author

apedroferreira commented Sep 12, 2024

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.

@apedroferreira apedroferreira merged commit 78ee244 into mui:master Sep 13, 2024
14 checks passed
@apedroferreira apedroferreira deleted the mini-drawer-sidebar branch September 13, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Components Button, input, table, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AppLayout] Support navigation mini drawer
3 participants