-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
MudDrawer: Fix initialization behaviour, use ParameterState, remove PreserveOpenState #8833
Conversation
Would be nice, if someone is willing to test it with their drawers. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #8833 +/- ##
==========================================
+ Coverage 89.82% 90.13% +0.30%
==========================================
Files 412 421 +9
Lines 11878 12277 +399
Branches 2364 2429 +65
==========================================
+ Hits 10670 11066 +396
+ Misses 681 662 -19
- Partials 527 549 +22 ☔ View full report in Codecov by Sentry. |
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.
Moving the boolean expressions into their own methods resulted in much more readable code. Good job!
Added to v7.0.0 Migration Guide #8447 |
Just upgraded to 7 preview 2 and now when I toggle light/dark mode my drawer closes. I believe this pr is the change that is causing this. I will try to make a repo to show the issue @ScarletKuro |
Sure, we will wait for repo, as I couldn't reproduce it on dev.mudblazor.com (it uses latest changes) that also has a drawer and dark/light switch. |
@ScarletKuro Thanks for looking into this. It seems to be related to the Repo: My real project (which is not public) has a mud drawer setup more like this, but I removed that complexity from the demo repo:
|
Hi, I've reviewed your repo, and I believe it now functions as expected. The previous behavior seemed incorrect. There are two main issues with your code:
It may not be easy to explain, but I'll start with the Even before my changes, the drawer's breakpoint comparison was using Previously, the drawer wasn't closing because the initial behavior wasn't functioning correctly. Since you're using a Since you're not using two-way binding, the drawer wants to close, but you have a variable with an initial value of "true" keeping the drawer open. However, when you click the theme toggle, it internally calls So the main reasons it was working before are:
If you change to Perhaps we should consider making changes from our side to handle |
I added a PR #8941 that makes Drawer to understand to work not only with |
@ScarletKuro |
Description
Replaces PR: #5012
Fixes: #4585
Fixes: #7273
Fixes: #4535
If you open the docs in small initial size, and start to resize it (large direction), the Drawer will not open. This should fix it.
Added our ParameterState framework.
I removed
PreserveOpenState
, it doesn't seems useful anymore, it looks like #684 (comment) it was just some property to overcome some implementation problems.Now when the
_isOpenWhenLarge
is gone (it was the main culprit why this whole thing wouldn't work adequately) thePreserveOpenState
is the default behavior now because it stays open when you increase the browser size:MudBlazor/src/MudBlazor/Components/Drawer/MudDrawer.razor.cs
Lines 317 to 321 in fa7d7e8
As I understand the
_isOpenWhenLarge
existed to restore the state when you lower window from a large window size and then resize it back then it goes to the "original" state before the resize. For example, if you had a large window and drawer closed (manually), then make the window small and back large again the drawer doesn't open, thePreserveOpenState
was there to disable this this restore logic, but it doesn't help with initial behavior. This in fact all conflicts with the initial behavior when you start with a small window, the_isOpenWhenLarge
is triggering when you resize to small window, but you already started small so it's just doesn't do anything. I removed all this logic, as it hard for me to find the justification for this default behavior, and there were no test coverage for it, all responsive tests were done againstPreserveOpenState=true
.The logic is simple, if it doesn't meet the breakpoints condition the drawer closes, and it does it opens, no "smart" features like restore state from large window etc.
I tried to chunk the code to small methods so that the main logic would look like this:
Also it seems like the missing popover provider was the reason why examples wouldn't work locally in Edge as I complained in discord.
How Has This Been Tested?
new bUnit tests Visual
Type of Changes
Checklist
dev
).