Drawer updates #684

tungi52 · 2021-01-27T09:33:31Z

Update drawer implementation and examples trying to cover some of requests and reported bugs. I'm pretty sure this PR doesn't solve all the problems, but I think it could be a good start. Important changes:

Add temporary, responsive and persistent drawer variants
Change Appbar and MainContent trying to make drawer a bit independent from MudLayout and make it possible to use inside a custom container
Updated examples to show the main new features
Updated ResizeListenerService to notify only when breakpoint changes
Define breakpoint sizes as variables in css - they are used in lot of components, so I think we an benefit from this approach

@Garderoben @henon @mikes-gh Can you take a look at it and test it? I highly appreciate your help.

Breaking changes:

Clipped parameter is replaced with ClipMode enum. Clipped="false" -> ClipMode="@ClipMode.Never", Clipped="true" -> ClipMode="@ClipMode.Docked"

mikes-gh · 2021-01-27T10:02:26Z

@tungi52 Looks like you've been busy. 👍 Ill take a look now.

mikes-gh · 2021-01-27T10:48:01Z

@tungi52 This is looking great. 👍

First off I am not a drawer expert. But that may be an advantage in helping us make these drawers as understandable as possible to the end user. As a start could you add a bit more description to the examples so I am completely clear on behaviours for each type of drawer. Also I think the temporary drawer should close on clicking an item within for demonstration purposes.

This is question - In clipped mode should the drawer overlap the main content rather than push it over. This may be by design so maybe this is just for my understanding.

tungi52 · 2021-01-27T10:56:34Z

@mikes-gh Thanks. You are right, docs should be improved, they are very minimal now.
In clipped mode currently I set up a persistent drawer, so I think this is a correct a behaviour. But waiting for other reviews.

mikes-gh · 2021-01-27T10:58:24Z

@tungi52 Yes it will help me review if the docs are done first as they are a design document. After that we can get into a code review

mikes-gh · 2021-01-27T16:07:42Z

Hi @tungi52 sorry for slow reply had other work to do. Looking good. I understand all the concepts. I did have to look up how clipped worked in Material UI. but that was it. I would expect responsive drawer to be clipped by default but it seems an anti-pattern to have a boolean that is true default. Others may have an opinion but its just a setting at the end of the day. I am going to start to dig into the code now.

henon · 2021-01-27T17:25:37Z

it seems an anti-pattern to have a boolean that is true default

I don't think so. A good default value (no matter the value) should be what most will expect when they don't touch the setting

mikes-gh · 2021-01-27T20:14:09Z

@tungi52
You've done a lot of work here. I think it looks to me like it's all working as designed.
One of the things that made drawer awkward was the interdependencies.
So DrawerContainer is the new go between for for Drawer and Layout?
But it doesn't decouple drawer from layout. Can drawer not exist without Layout or DrawerContainer

Just exploring this 😄

mikes-gh · 2021-01-27T20:36:01Z

I realise you are fixing everyone else's design here but one thing I really thought should be addressed is that MudNavLink is still in direct communication with the drawer.
https://github.com/Garderoben/MudBlazor/blob/5100566e7b1b7e987a8ace2469f0f114f1c6658a/src/MudBlazor/Components/NavMenu/MudNavLink.razor.cs#L28
I would say MudNavMenu should be raising an event that MudDrawer can subscribe @henon could I ask for your input please

tungi52 · 2021-01-27T20:38:51Z

@mikes-gh I admit that this is the most problematic part of the code. MainContent and AppBar should be changed depends on the drawer state. This is part of the current behaviour, so I wanted to keep it. As you wrote, DrawerContainer is the new link between Drawer, Layout, AppBar and MainContent and this link provide this functionality. But this dependency is not necessary, with Fixed parameter one can decouple it from the Layout and use in any container (this only works for persistent drawers at the moment - I don't know for others it has a meaning at all).

mikes-gh · 2021-01-27T21:22:32Z

@tungi52 OK lets leave aside the Layout App bar dependencies for now. Could you decouple NavLink please and make a callback in NavMenu. NavMenu or any of its constituents definitely shouldnt know about drawer.

tungi52 · 2021-01-27T21:32:12Z

@tungi52 OK lets leave aside the Layout App bar dependencies for now. Could you decouple NavLink please and make a callback in NavMenu. NavMenu or any of its constituents definitely shouldnt know about drawer.

Ok, I understand. Can you give me an advice? In general Drawer does not know there is NavMenu inside it so I have no idea how to start. How a drawer should register to an event from NavMenu? Or cascading some common object instead of the drawer itself?

mikes-gh · 2021-01-27T21:36:41Z

NavMenu would have a callback Parameter OnNavigate.
Drawer would provide the delegate that gets called which would have your openclose logic etc
Chris Sainty does a good job of explaining this.
https://chrissainty.com/3-ways-to-communicate-between-components-in-blazor/

mikes-gh · 2021-01-27T21:50:13Z

I guess its up-to the the developer to wire this up. Best demonstrated in a sample. But NavLink shouldn't be calling drawer directly. Since Drawer is often going to have NavMenu inside we just need to make sure its easy to wire them up.

tungi52 · 2021-01-27T21:51:02Z

Yes I know this, a really helpful write. But for me a combination of option 2 and 3 could work (or make a new service?). Point 1 is not possible, because Drawer and NavMenu don't know about each other directly. Only in the user code, but I think the idea behind the current solution was that on navigation the drawer should close automatically. Or did I misunderstand you?

mikes-gh · 2021-01-27T21:52:27Z

Yes its just an open close thing.

mikes-gh · 2021-01-27T21:55:54Z

I think we just need a sample showing it as its a common senario and get rid of the Drawer code in NavLink

mikes-gh · 2021-01-27T21:57:39Z

NavMenu and Drawer dont need to know about each other thats up to the developer IMO

henon · 2021-01-27T21:57:42Z

I am not sure. what about this: let drawer send down a cascaded value of INavigationEventReceiver or something similar and let NavLink call this on navigation? Other links, buttons etc could implement the same

mikes-gh · 2021-01-27T22:08:21Z

@henon Do you think this is not a simple docs exercise? Plus the developer may want to do different things on navigate. as well as close the drawer (or keep it open). Do you think we need a default behaviour for NavMenu inside drawer?
Just some things to consider. The delegate for OnNavigate could be a s simple as OnNavigate => this.Close();. Thats almost config rather than code.

henon · 2021-01-27T22:49:43Z

Ok, you have a point there, Mike.

mikes-gh · 2021-01-27T22:59:37Z

Just putting it out there. Always open to ideas. As always its a balance between complete flexibility and making it easy for simple senarios. I guess there are lots of senarios of what drawer could contain. It could be like a complete reporting parameter area so I dont think we should try to tie this together. For this simple senario of NavMenu closes opens the drawer we can show in the docs I think.

tungi52 · 2021-01-28T09:51:26Z

I pushed 2 commits. The first one contains some additional features requested in #388 and #627. I replaced the Clipped parameter with an enum, so now there are more options to control this behaviour. Here I can accept some other ideas for naming :)
In the second one I decoupled NavLink, so in the future developers should take aware of this.
In the initial comment I started to collect the breaking changes.

mikes-gh · 2021-01-28T10:36:02Z

@tungi52 if you're doing any docs be aware of this https://github.com/Garderoben/MudBlazor/pull/688

mikes-gh

@tungi52 Thats my first scan. Ive got todo some other bits now. There is a great deal of work in this PR thank you. Apologies if Im dipping in and out of it.

src/MudBlazor/Components/NavMenu/MudNavMenu.razor

src/MudBlazor.Docs/Pages/Components/Drawer/Examples/DrawerClippingExample.razor

henon · 2021-01-28T13:07:29Z

Is the breakpoint for responsive drawer configurable? Are the two special breakpoints Breakpoint.None and Breakpoint.Always also supported? They sound crazy but they actually make sense sometimes, i.e. if you want to force smallscreen behavior even if the screen is big or if you want to suppress it even if the screen is small.

Does it make sense or not?

tungi52 · 2021-01-28T14:19:25Z

@henon I think it is not supported as ealier, but there new variants (temporary, persistent) which are not depends on the screen size. IMO this solve these issues and developers can find the best one which fits their needs. Also I planning other variants too (mini, floating) but later.

henon

I made an API review so we keep all publicly accessible functions consistent with the rest of the lib. After applying these little changes I think we can merge or what do you think @tungi52 ?

src/MudBlazor/Components/Drawer/MudDrawer.razor.cs

src/MudBlazor/Services/ResizeListener/ResizeListenerService.cs

src/MudBlazor/Services/ResizeListener/ResizeOptions.cs

mikes-gh · 2021-01-28T22:34:52Z

@tungi52 Looking close now.
Can you look at the responsive examples please and the custom break point. I am not sure they are working as designed but it may be my understanding

set the responsive drawer to a small screen size. Open a drawer which comes up temporary as expected. Make the window bigger past the break point and the drawer closes. To me this is un expected.
With the custom breakpoints I am also getting un expected drawer state when resizing.
Could you check and let me know. It could be me 😄

mikes-gh · 2021-01-28T22:39:15Z

Also there is some grammar in the examples that needs changing. Obviously this is much harder if english is not your first language so one of us can do this after the merge. Just flagging it here as a TODO

mikes-gh · 2021-01-28T22:49:56Z

I see @tungi52 has looked at MockResizeListenerService.cs for tests. I have no idea where to start with writing tests for this. :-(. Just flagging this also. I am sure @henon has some idea.

tungi52 · 2021-01-29T07:30:46Z

@tungi52 Looking close now.
Can you look at the responsive examples please and the custom break point. I am not sure they are working as designed but it may be my understanding

set the responsive drawer to a small screen size. Open a drawer which comes up temporary as expected. Make the window bigger past the break point and the drawer closes. To me this is un expected.

With the custom breakpoints I am also getting un expected drawer state when resizing.

Could you check and let me know. It could be me 😄

Ok this is a design question I think. Currently responsive drawer saves the large state, which is null by default. This help to restore the original state after resize (I think gitter works similarly). So for now this is "expected", but I understand what your problem with it. Maybe a parameter like PreserveOpenState? @henon

mikes-gh · 2021-01-29T07:48:17Z

@tungi52 Gitter actually closes the drawer on mouse out so by the time you goto make the screen bigger the drawer is already closed.

tungi52 · 2021-01-29T07:54:16Z

@mikes-gh I think I change it like the saved state only matters when we resize from small to large and the drawer is closed.

mikes-gh · 2021-01-29T07:58:44Z

@tungi52 @henon I would get some other opinions before you change it.

henon · 2021-01-29T10:22:28Z

PreserveOpenState

Sure, this is a very good idea! I am pretty sure our users will want both, so making it configurable is always the best option.

henon · 2021-01-29T10:27:41Z

@mikes-gh: in MockResizeListener we can add functions to initiate virtual screen size/breakpoint change events from within the test case. It should then be easy to check if certain CSS classes were applied as expected.

mikes-gh · 2021-01-29T17:50:27Z

@tungi52 Do you think this is ready now? Seems good to me. We can write the tests and tighten up grammar on the docs later.

Garderoben · 2021-01-30T16:48:44Z

Can we merge it today you think @henon @tungi52 ?

tungi52 · 2021-01-30T17:06:03Z

@Garderoben I think it is ready.

henon · 2021-01-30T17:24:10Z

Thanks a ton for this great work @tungi52 !

mikes-gh · 2021-01-30T19:01:16Z

Thanks @tungi52 this was a lot of work 👍

leMicin · 2021-01-30T19:08:58Z

It looks like there is an issue with the generated class name and what is in the compiled css.

With the following code, the class '.mud-main-drawer-open-persistent-md-left' is put on the '.mud-main-content'. That class does nothing. When I looked in the PR code, I found that removing the '-md' fixed it for me. I tried various Breakpoint settings and they all have this issue.

<MudLayout>
    <MudAppBar></MudAppBar>

    <MudDrawer ClipMode="DrawerClipMode.Always"
               Open="true"
               Variant="DrawerVariant.Persistent">
        <MudNavMenu>
            <MudNavLink Match="NavLinkMatch.All" Href="/settings">@Localizer[SettingsResources.General]</MudNavLink>
            <MudNavLink Match="NavLinkMatch.All" Href="/settings/price">@Localizer[SettingsResources.Price]</MudNavLink>
            <MudNavLink Match="NavLinkMatch.All" Href="/settings/map">@Localizer[SettingsResources.Map]</MudNavLink>
            <MudNavLink Match="NavLinkMatch.All" Href="/settings/wiki">@Localizer[SettingsResources.Wiki]</MudNavLink>
            <MudNavLink Match="NavLinkMatch.All" Href="/settings/chat">@Localizer[SettingsResources.Chat]</MudNavLink>
            <MudNavLink Match="NavLinkMatch.All" Href="/settings/stash">@Localizer[SettingsResources.Stash]</MudNavLink>
        </MudNavMenu>
    </MudDrawer>

    <MudMainContent>
        <MudContainer>
            @ChildContent
        </MudContainer>
    </MudMainContent>
</MudLayout>

.mud-main-drawer-open-persistent-md-left

.mud-main-drawer-open-persistent-left

henon · 2021-01-30T19:12:35Z

@tungi52 I pre-released the drawer in https://www.nuget.org/packages/MudBlazor/1.2.4.12-dev which is what leMicin used. Maybe I made an error when packaging?

mikes-gh · 2021-01-30T19:31:13Z

Maybe I made an error when packaging?

@henon if you used dotnet pack I dont see how. Are we saying this could be an scss compiler issue?

tungi52 · 2021-01-30T19:35:54Z

no, no it's my fault. I found a small problem in css. How can I fix it? Pushing a new commit in this PR is ok?

henon · 2021-01-30T19:36:45Z

make a new PR and link it here

leMicin · 2021-01-30T19:37:13Z

@mikes-gh , When I look at the scss code inside src/MudBlazor/Styles/layout/_main.scss (lines: 33-48). It really looks like there is not class name that includes the breakpoint in it.

The GetDrawerClass() method in src/MudBlazor/Components/Main/MudMainContent.razor does include the breakpoint in the compiled class name.

There is a mismatch between the two, which is what I tried to explain with pictures.

henon · 2021-01-31T08:03:49Z

@leMicin the css fixes are in pre-release https://www.nuget.org/packages/MudBlazor/1.2.4.13-dev

Drawer updates

bdb15c8

docs update

7bef0af

tungi52 added 2 commits January 28, 2021 08:19

add some new feature to drawer

12ab14f

decouple MudNavLink

73d7523

mikes-gh reviewed Jan 28, 2021

View reviewed changes

henon requested changes Jan 28, 2021

View reviewed changes

change default breakpoint, fix namings

58e686d

henon self-requested a review January 29, 2021 13:15

henon approved these changes Jan 29, 2021

View reviewed changes

tungi52 marked this pull request as ready for review January 29, 2021 19:37

henon merged commit b80d1f3 into MudBlazor:develop Jan 30, 2021

tungi52 mentioned this pull request Jan 30, 2021

Fix drawer css #700

Merged

tungi52 deleted the drawer-update branch February 1, 2021 17:55

ScarletKuro mentioned this pull request Apr 29, 2024

MudDrawer: Fix initialization behaviour, use ParameterState, remove PreserveOpenState #8833

Merged

7 tasks

Drawer updates #684

Drawer updates #684

Conversation

tungi52 commented Jan 27, 2021 • edited

mikes-gh commented Jan 27, 2021

mikes-gh commented Jan 27, 2021 • edited

tungi52 commented Jan 27, 2021

mikes-gh commented Jan 27, 2021

mikes-gh commented Jan 27, 2021

henon commented Jan 27, 2021

mikes-gh commented Jan 27, 2021

mikes-gh commented Jan 27, 2021

tungi52 commented Jan 27, 2021

mikes-gh commented Jan 27, 2021 • edited

tungi52 commented Jan 27, 2021

mikes-gh commented Jan 27, 2021

mikes-gh commented Jan 27, 2021

tungi52 commented Jan 27, 2021 • edited

mikes-gh commented Jan 27, 2021

mikes-gh commented Jan 27, 2021

mikes-gh commented Jan 27, 2021

henon commented Jan 27, 2021

mikes-gh commented Jan 27, 2021

henon commented Jan 27, 2021

mikes-gh commented Jan 27, 2021 • edited

tungi52 commented Jan 28, 2021

mikes-gh commented Jan 28, 2021

mikes-gh left a comment

Choose a reason for hiding this comment

henon commented Jan 28, 2021

tungi52 commented Jan 28, 2021

henon left a comment

Choose a reason for hiding this comment

mikes-gh commented Jan 28, 2021 • edited

mikes-gh commented Jan 28, 2021

mikes-gh commented Jan 28, 2021

tungi52 commented Jan 29, 2021

mikes-gh commented Jan 29, 2021

tungi52 commented Jan 29, 2021

mikes-gh commented Jan 29, 2021 • edited

henon commented Jan 29, 2021 • edited

henon commented Jan 29, 2021 • edited

mikes-gh commented Jan 29, 2021

Garderoben commented Jan 30, 2021

tungi52 commented Jan 30, 2021

henon commented Jan 30, 2021

mikes-gh commented Jan 30, 2021

leMicin commented Jan 30, 2021 • edited

henon commented Jan 30, 2021

mikes-gh commented Jan 30, 2021

tungi52 commented Jan 30, 2021

henon commented Jan 30, 2021

leMicin commented Jan 30, 2021

henon commented Jan 31, 2021

tungi52 commented Jan 27, 2021 •

edited

mikes-gh commented Jan 27, 2021 •

edited

mikes-gh commented Jan 27, 2021 •

edited

tungi52 commented Jan 27, 2021 •

edited

mikes-gh commented Jan 27, 2021 •

edited

mikes-gh commented Jan 28, 2021 •

edited

mikes-gh commented Jan 29, 2021 •

edited

henon commented Jan 29, 2021 •

edited

henon commented Jan 29, 2021 •

edited

leMicin commented Jan 30, 2021 •

edited