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

MudThemeProvider: Fix BL0007, avoid direct parameter writes, remove MudThemingProvider, and other improvements #8712

Merged
merged 17 commits into from
Apr 22, 2024

Conversation

ScarletKuro
Copy link
Member

@ScarletKuro ScarletKuro commented Apr 15, 2024

Description

IsDarkMode is using ParamterState framework.
I couldn't really do the same for Theme as I don't think BuildTheme can be made async because of this


So I ended up doing in old fashion. Let me know if I'm wrong.
watchDarkThemeMedia is now automatically subscribed on first render, it's necessary because otherwise two way binding IsDarkMode will never ever be called if you don't call WatchSystemPreference() which I don't think is a good default behavior, otherwise the IsDarkMode shouldn't be two way bindable at all.

Added JS stopWatchingDarkThemeMedia to unsubscribe from the event (before it never was), I know there is only one theme provider during the lifetime, but I think when you use automode you will end up having two subscription and this is not desired (but this is theory, I'm not an auto user), it doesn't bring any harm, just allocated additional memory if it stays.
Did an overall cleanup of the JS script:

  1. The darkThemeMediaQuery was defined, yet only used for single function, when it could be reused.
  2. The dotNetHelper in darkModeChange wasn't used, it's not needed.
  3. The e wasn't used, which is a small mistake, because you are invoking the matchMedia once again, when you already got a result.

How Has This Been Tested?

Visually, but just docs, I don't know if i should tests some other scenarios? pls, let me know
New simple bUnit tests.

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.

@github-actions github-actions bot added bug Something does not work as intended/expected PR: needs review labels Apr 15, 2024
Copy link

codecov bot commented Apr 15, 2024

Codecov Report

Attention: Patch coverage is 98.03922% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 90.09%. Comparing base (28bc599) to head (b638bbb).
Report is 90 commits behind head on dev.

Files Patch % Lines
...Components/ThemeProvider/MudThemeProvider.razor.cs 98.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8712      +/-   ##
==========================================
+ Coverage   89.82%   90.09%   +0.26%     
==========================================
  Files         412      419       +7     
  Lines       11878    12190     +312     
  Branches     2364     2400      +36     
==========================================
+ Hits        10670    10982     +312     
+ Misses        681      668      -13     
- Partials      527      540      +13     

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

@ScarletKuro ScarletKuro changed the title MudThemingProvider: Fix BL0007, not write to parameter directly, othe… MudThemingProvider: Fix BL0007, not write to parameter directly, other improvements Apr 15, 2024
Copy link
Contributor

@danielchalmers danielchalmers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran it on my prod app that has custom colors and System/Light/Dark modes and all seems the same

@ScarletKuro
Copy link
Member Author

Removed MudThemeProvider.
Let me know if we need to have a better naming for the MudThemingProvider.

@mikes-gh
Copy link
Contributor

Removed MudThemeProvider. Let me know if we need to have a better naming for the MudThemingProvider.

As discussed lets name it to the old name MudThemeProvider and document the removal of MudPopoverProvider. Might be easier in a separate PR IDK

Copy link
Contributor

@mikes-gh mikes-gh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ScarletKuro thanks . Probably need to add <MudPopoverProvider/> in a few places

@ScarletKuro
Copy link
Member Author

I think that's all places, let me know if I missed something,

@mikes-gh
Copy link
Contributor

@ScarletKuro Thanks 👍

@mikes-gh
Copy link
Contributor

mikes-gh commented Apr 22, 2024

Maybe we should change the title for release notes?

MudThemeProvider: Fix BL0007, not write to parameter directly, remove MudThemingProvider and other improvements (#8712)?

@ScarletKuro ScarletKuro changed the title MudThemingProvider: Fix BL0007, not write to parameter directly, other improvements MudThemingProvider: Fix BL0007, avoid direct parameter writes, remove MudThemingProvider, and other improvements Apr 22, 2024
@ScarletKuro ScarletKuro changed the title MudThemingProvider: Fix BL0007, avoid direct parameter writes, remove MudThemingProvider, and other improvements MudThemeProvider: Fix BL0007, avoid direct parameter writes, remove MudThemingProvider, and other improvements Apr 22, 2024
@ScarletKuro ScarletKuro merged commit d6cbae0 into MudBlazor:dev Apr 22, 2024
4 checks passed
@ScarletKuro ScarletKuro deleted the themerprovider_fix branch April 22, 2024 17:17
@ScarletKuro
Copy link
Member Author

Added to v7.0.0 Migration Guide #8447

@henon
Copy link
Collaborator

henon commented Apr 23, 2024

Added Warning box to make it clear and not easily overlooked.
image

biegehydra pushed a commit to biegehydra/MudBlazor that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected PR: needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants