fix(DockView): theme parameter not work #5705
Conversation
Reviewer's Guide by SourceryThis pull request addresses an issue where the theme parameter was not functioning correctly in the DockView component. The changes involve refactoring theme handling to use the Sequence diagram for Theme ChangesequenceDiagram
participant User
participant ThemeMode
participant IThemeProvider
participant BaseDockView
User->ThemeMode: Click to change theme
ThemeMode->IThemeProvider: SetThemeAsync(themeName)
activate IThemeProvider
IThemeProvider->IThemeProvider: Store theme
IThemeProvider-->>ThemeMode: Return
deactivate IThemeProvider
IThemeProvider->BaseDockView: ThemeChangedAsync(themeName)
activate BaseDockView
BaseDockView->BaseDockView: Update Theme property
BaseDockView->BaseDockView: StateHasChanged()
BaseDockView-->>IThemeProvider: Return
deactivate BaseDockView
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a guard to
DefaultThemeProvider.csto preventThemeChangedAsyncfrom being called if it's null. - The asset path is hardcoded in
Pre.razor.js; consider making this configurable.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| DataTableDynamicContext = DataTableDynamicService.CreateContext(); | ||
|
|
||
| ThemeProviderService.ThemeChangedAsync += OnThemeChanged; |
There was a problem hiding this comment.
suggestion: Unsubscribe from ThemeChangedAsync to prevent potential memory leaks.
Since the component registers for theme change notifications during OnInitialized, consider unsubscribing in Dispose to ensure that disposed instances do not remain referenced through the delegate.
Suggested implementation:
base.OnInitialized();
ThemeProviderService.ThemeChangedAsync += OnThemeChanged; // Unsubscribe from the ThemeChangedAsync event to avoid memory leaks
public void Dispose()
{
if (ThemeProviderService != null)
{
ThemeProviderService.ThemeChangedAsync -= OnThemeChanged;
}
GC.SuppressFinalize(this);
}If your class already overrides a Dispose(bool disposing) method, then incorporate the unsubscription logic there instead of creating a new Dispose() method. Additionally, ensure that the class implements IDisposable if it isn’t already.
| { | ||
| base.OnInitialized(); | ||
|
|
||
| ThemeProviderService.ThemeChangedAsync += OnThemeChanged; |
There was a problem hiding this comment.
suggestion: Manage event subscription lifecycle in Pre component.
Just as in other components, ensure that the event handler registered to ThemeProviderService.ThemeChangedAsync is removed when the component is disposed. This helps avoid unintended behavior and memory retention.
Suggested implementation:
protected override void Dispose(bool disposing)
{
if (disposing)
{
ThemeProviderService.ThemeChangedAsync -= OnThemeChanged;
}
base.Dispose(disposing);
}Ensure that the method OnThemeChanged exists in the component. If not, implement it accordingly.
| /// <summary> | ||
| /// The callback when theme changed | ||
| /// </summary> | ||
| Func<string, Task>? ThemeChangedAsync { get; set; } |
There was a problem hiding this comment.
suggestion: Consider using an event instead of a delegate property for theme changes.
Using the event keyword and following the .NET event pattern could make subscribing and unsubscribing more natural, especially if multiple components might listen to theme change notifications.
| /// <summary> | |
| /// The callback when theme changed | |
| /// </summary> | |
| Func<string, Task>? ThemeChangedAsync { get; set; } | |
| /// <summary> | |
| /// Occurs when the theme is changed. | |
| /// </summary> | |
| event Func<string, Task>? ThemeChangedAsync; |
| await Task.CompletedTask; | ||
| }; | ||
| await themeProviderService.SetThemeAsync("light"); | ||
| Assert.Equal("light", themeName); |
There was a problem hiding this comment.
suggestion (testing): Add test for the ThemeChangedAsync event with "dark" theme
It would be beneficial to add a test case that specifically checks the behavior of the ThemeChangedAsync event when the "dark" theme is set. This ensures that the event is triggered correctly and the theme name is passed accurately in both light and dark scenarios.
| public async Task SetTheme_Ok() | ||
| { | ||
| var themeName = ""; | ||
| var themeProviderService = Context.Services.GetRequiredService<IThemeProvider>(); |
There was a problem hiding this comment.
suggestion (testing): Add test for GetThemeAsync
A test is missing to verify the functionality of the GetThemeAsync method. This test should set a theme using SetThemeAsync and then retrieve it using GetThemeAsync, asserting that the retrieved theme matches the set theme.
Suggested implementation:
[Fact]
public async Task SetTheme_Ok()
{
var themeName = "";
var themeProviderService = Context.Services.GetRequiredService<IThemeProvider>();
themeProviderService.ThemeChangedAsync = async theme =>
{
themeName = theme;
await Task.CompletedTask;
};
await themeProviderService.SetThemeAsync("light");
Assert.Equal("light", themeName);
}
[Fact]
public async Task GetTheme_Ok()
{
// Arrange
var themeProviderService = Context.Services.GetRequiredService<IThemeProvider>();
await themeProviderService.SetThemeAsync("light");
// Act
var theme = await themeProviderService.GetThemeAsync();
// Assert
Assert.Equal("light", theme);
}This change assumes that the IThemeProvider interface includes a GetThemeAsync method that functions synchronously after setting the theme. If additional setup is required in your test fixture or dependency injection container, please ensure to update that accordingly.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5705 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 657 657
Lines 29818 29825 +7
Branches 4225 4226 +1
=========================================
+ Hits 29818 29825 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Link issues
fixes #5686
Summary By Copilot
This pull request includes several changes to the
BootstrapBlazor.Serverproject, focusing on theme management and dependency updates. The most important changes include updating package versions, refactoring theme handling, and modifying various components to support dynamic theme changes.Dependency Updates:
BootstrapBlazor.DockViewpackage version from9.1.5to9.1.7inBootstrapBlazor.Server.csproj.Theme Handling Refactor:
OnThemeChangedAsyncmethod and its usage fromHeader.razorandHeader.razor.cs. [1] [2]Pre.razor.csto injectIThemeProviderand handle theme changes using theOnThemeChangedmethod. [1] [2] [3]ThemeMode.razorandThemeMode.razor.csto useJSObjectReferenceand handle theme changes withOnThemeChangedmethod. [1] [2] [3] [4]BaseDockView.csto injectIThemeProviderand handle theme changes by updating theThemeproperty. [1] [2] [3] [4]Component Updates:
Themeparameter to variousDockViewV2components in different.razorfiles to support dynamic theme changes. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]JavaScript Changes:
updateThemefunction and its usage fromHeader.razor.js. [1] [2]switchThemefunction inPre.razor.jsby removing theassetPathparameter.ThemeMode.razor.jsto invoke theOnThemeChangedmethod when the theme changes. [1] [2]Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Fixes an issue where the theme parameter was not working in the DockView component. This is achieved by refactoring theme handling to use the IThemeProvider service and updating components to support dynamic theme changes.
Bug Fixes:
Enhancements:
Tests: