MudDrawer: Make Temporary Drawer non-responsive

[Nickztar]

Description

Not setting a breakpoint with a Temporary drawer as auto-open (_open being true on initial render) should still open on smaller devices. In a larger window this opens automatically, but when scaled down. It doesn't actually open, due to the immediate response from ViewportService.

Possible work-arounds:
Setting the breakpoint parameter on drawer to something like Breakpoint.Xs does give expected result but does feel like a weird problem since the breakpoint parameter is documented to only be related to resposive drawers.

Other solutions considered:
Setting breakpoint to Breakpoint.Always but then _open isnt respected (So having it NOT auto open doesnt work. It always opens). Another solution that might have been possible is Breakpoint.None, but then we are back at not respecting _open for auto-opening.

A short example with some notes in TryMudblazor (https://try.mudblazor.com/snippet/caGobEGTREtbcRBn)

How Has This Been Tested?

A new unit test has been added, which checks to make sure that for all the breakpoints returned from Viewport service, the initial state set in the drawer is kept, if the drawer is not response/mini.

Type of Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (fix or improvement to the website or code docs)

Checklist

• The PR is submitted to the correct branch (dev).
• My code follows the code style of this project.
• I've added relevant tests.

[Nickztar]

Actually not sure if this is a breaking change. This is how I would expect it to work when not responsive. But could be a change for someone that expects the Breakpoints to control their non-response drawer?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.16%. Comparing base (640b9be) to head (bfa8d0b).
Report is 2 commits behind head on dev.

Additional details and impacted files

@@           Coverage Diff           @@
##              dev   #10095   +/-   ##
=======================================
  Coverage   91.16%   91.16%           
=======================================
  Files         411      411           
  Lines       12468    12467    -1     
  Branches     2420     2420           
=======================================
  Hits        11366    11366           
  Misses        557      557           
+ Partials      545      544    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ScarletKuro]

Hi.

Thanks for the PR. I guess your change makes sense for Temporary drawer.
It also doesn't change any responsive logic.
Just will get second opinion @henon just in case.

[ScarletKuro]

LGTM

[henon]

For me the docs about temporary drawers are unclear. It being above all other content is clear.

Temporary Drawers can be opened temporarily above all other content until a section is selected, or until the overlay is clicked if OverlayAutoClose is set to true.

Then later in the docs of other drawers the word temporarily is used to describe responsive behavior, meaning auto opening / closing based on breakpoint.

So I am not sure if it is correct to completely remove breakpoint update notification from temporary drawers. Or am I getting it all wrong?

[ScarletKuro]

Then later in the docs of other drawers the word temporarily is used to describe responsive behavior, meaning auto opening / closing based on breakpoint.

So I am not sure if it is correct to completely remove breakpoint update notification from temporary drawers. Or am I getting it all wrong?

Honestly, for clarification, we would need @tungi52, but I don't think we’ll get a reply. I refactored most of the drawer, but I never figured out how the Temporary drawer is actually supposed to work, so the logic for that part was left untouched and most if not all tests are against responsive drawer. From what I can see in the issues, people do not expect Temporary and Persistent to depend on breakpoints at all, because otherwise, you will have poor control over it with the Open property.

[henon]

I see. I guess we should then clarify this in the documentation. Merging this in v8 is perfect, we can clarify in the migration guide that the behavior of the Temporary Drawer has now been officially defined to be non-responsive.

Text for the v8 migration guide:

• MudDrawer: The variant Temporary has been changed to behave non-responsively, meaning solely controlled by the Open parameter. #10095

@Nickztar can you update the documentation in this PR so it is perfectly clear? Then we can merge.

[henon]

If anyone wants it to be responsive we can always add another parameter to MudDrawer, i.e. bool Responsive that enables or disables responsiveness according to breakpoint settings.

[Nickztar]

Sounds good. I will try to add some documentation to make it clearer to users what and what isn't responsive.

Adding a Responsive parameter would be very simple aswell since the check for using the breakpoints is the same method for initial rendering as it is for changes. 👍🏼

[Nickztar]

@henon Guess this clarifies it a bit. Let me know if you have any ideas on other parts where it can be clarified.

[henon]

@Nickztar Check it out, I modified the text of all drawer variants.

[Nickztar]

Looks great. Much clearer how the other types work aswell!

[ScarletKuro]

LGTM, @henon feel free to merge.

[henon]

Thanks @Nickztar!

[ScarletKuro]

Added to the v8 migration guide at #9953