-
-
Notifications
You must be signed in to change notification settings - Fork 367
feat(EditorForm): add IsRenderWhenValueChanged parameter #7138
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
Conversation
Reviewer's GuideThis PR extends EditorForm to support optional live re-rendering on field value changes by subscribing to EditContext.OnFieldChanged, ensures proper disposal of subscriptions, enables the feature in EditDialog, and updates the TablesEdit sample with a custom EditTemplate while removing obsolete components. Sequence diagram for EditorForm live re-rendering on field value changesequenceDiagram
participant EditContext
participant EditorForm
EditContext->>EditorForm: OnFieldChanged event
EditorForm->>EditorForm: NotifyValueChanged()
alt IsRenderWhenValueChanged == true
EditorForm->>EditorForm: StateHasChanged() (re-render)
end
Class diagram for updated EditorForm with IsRenderWhenValueChanged and IDisposableclassDiagram
class EditorForm_TModel {
+bool IsRenderWhenValueChanged
+void Dispose()
-void NotifyValueChanged(object? sender, FieldChangedEventArgs args)
}
EditorForm_TModel --|> IDisposable
EditorForm_TModel --|> IShowLabel
Class diagram for removal of DemoTableEditTemplate componentclassDiagram
class DemoTableEditTemplate {
<<removed>>
}
Class diagram for TablesEdit sample with custom EditTemplate and helper methodclassDiagram
class TablesEdit {
-string? GetEducationDesc(EnumEducation? item)
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/EditorForm/EditorForm.razor.cs:15` </location>
<code_context>
/// </summary>
[CascadingTypeParameter(nameof(TModel))]
-public partial class EditorForm<TModel> : IShowLabel
+public partial class EditorForm<TModel> : IShowLabel, IEditorFormValueChanged
{
private string? ClassString => CssBuilder.Default("bb-editor")
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the custom value change interface and method with a direct EditContext.OnFieldChanged subscription for automatic re-rendering.
```markdown
You can drop the new interface + `NotifyValueChanged()` and just hook into the existing `EditContext.OnFieldChanged` to trigger `StateHasChanged()`. This removes the indirection and keeps all functionality.
1. Remove
- `IEditorFormValueChanged` from the class signature
- the `IsRenderWhenValueChanged` parameter
- the `NotifyValueChanged()` method
2. Add an `OnInitialized` (or `OnParametersSet`) subscription:
```csharp
public partial class EditorForm<TModel> : IShowLabel, IDisposable
{
[Parameter]
public bool AutoRefreshOnFieldChanged { get; set; }
[CascadingParameter] private EditContext? CascadedEditContext { get; set; }
private EventHandler<FieldChangedEventArgs>? _fieldChangedHandler;
protected override void OnInitialized()
{
if (AutoRefreshOnFieldChanged && CascadedEditContext != null)
{
_fieldChangedHandler = (_, __) => InvokeAsync(StateHasChanged);
CascadedEditContext.OnFieldChanged += _fieldChangedHandler;
}
}
public void Dispose()
{
if (_fieldChangedHandler != null && CascadedEditContext != null)
{
CascadedEditContext.OnFieldChanged -= _fieldChangedHandler;
}
}
}
```
3. Update your consumers to use `AutoRefreshOnFieldChanged` instead of `IsRenderWhenValueChanged`.
This way you get the same re-render-on-value-change behavior without a custom interface or manual `NotifyValueChanged()` calls.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7138 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 745 745
Lines 32548 32566 +18
Branches 4510 4513 +3
=========================================
+ Hits 32548 32566 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR adds the IsRenderWhenValueChanged parameter to the EditorForm component, enabling automatic re-rendering when child component values change. This addresses issue #7137 by allowing form components to update reactively without requiring manual component encapsulation.
Key Changes:
- Introduces
IEditorFormValueChangedinterface with aNotifyValueChanged()method for value change notifications - Adds
IsRenderWhenValueChangedparameter toEditorForm(default:false) to control re-rendering behavior - Updates
ValidateBaseto notify parentEditorFormwhen values change through cascading parameters - Simplifies demo code by removing the need for separate component encapsulation (
DemoTableEditTemplate) to achieve reactive updates
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/EditorForm/IEditorFormValueChanged.cs | New interface defining the value change notification contract |
| src/BootstrapBlazor/Components/Validate/ValidateBase.cs | Adds cascading parameter to receive EditorForm instance and notify on value changes |
| src/BootstrapBlazor/Components/EditorForm/EditorForm.razor.cs | Implements IEditorFormValueChanged interface and adds IsRenderWhenValueChanged parameter |
| src/BootstrapBlazor/Components/Dialog/EditDialog.razor | Sets IsRenderWhenValueChanged="true" to enable reactive updates in edit dialogs |
| src/BootstrapBlazor.Server/Locales/zh-CN.json | Updates documentation text to reflect version 10.0.1 supporting reactive functionality |
| src/BootstrapBlazor.Server/Components/Samples/Table/TablesEdit.razor | Refactors example to use inline EditTemplate instead of separate component |
| src/BootstrapBlazor.Server/Components/Samples/Table/TablesEdit.razor.cs | Moves education description logic from separate component to main page |
| src/BootstrapBlazor.Server/Components/Components/DemoTableEditTemplate.razor.cs | Removes obsolete separate component (no longer needed) |
| src/BootstrapBlazor.Server/Components/Components/DemoTableEditTemplate.razor | Removes obsolete separate component template |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/BootstrapBlazor/Components/EditorForm/IEditorFormValueChanged.cs
Outdated
Show resolved
Hide resolved
|
@sourcery-ai review |
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor.Server/Components/Samples/Table/TablesEdit.razor:61-70` </location>
<code_context>
ShowToolbar="true" ShowExtendButtons="true" ShowSkeleton="true">
<TableColumns>
<TableColumn @bind-Field="@context.Complete" Align="Alignment.Center"></TableColumn>
+ <TableColumn @bind-Field="@context.Education">
+ <EditTemplate Context="v">
+ <div class="col-12 col-sm-6">
+ </div>
+ <div class="col-12 col-sm-6">
+ <Select @bind-Value="@v.Education"></Select>
+ </div>
+ <div class="col-12 col-sm-6">
+ <Display Value="@GetEducationDesc(v.Education)" ShowLabel="true" DisplayText="@Localizer["TablesEditTemplateDisplayLabel"]"></Display>
+ </div>
+ </EditTemplate>
</code_context>
<issue_to_address>
**suggestion:** EditTemplate for Education column contains empty divs.
Please remove or merge the empty grid divs to simplify the markup and enhance code clarity.
```suggestion
<EditTemplate Context="v">
<div class="col-12 col-sm-6">
<Select @bind-Value="@v.Education"></Select>
</div>
<div class="col-12 col-sm-6">
<Display Value="@GetEducationDesc(v.Education)" ShowLabel="true" DisplayText="@Localizer["TablesEditTemplateDisplayLabel"]"></Display>
</div>
</EditTemplate>
```
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Components/EditorForm/EditorForm.razor.cs:162` </location>
<code_context>
-
-namespace BootstrapBlazor.Server.Components.Components;
-
-/// <summary>
-/// DemoTableEditTemplate component
-/// </summary>
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the Dispose implementation by removing the virtual Dispose(bool) method and finalizer suppression, and only unsubscribing in IDisposable.Dispose.
```csharp
// Drop the full Dispose pattern (no finalizer, no virtual Dispose(bool))
// and implement IDisposable.Dispose directly to just unsubscribe:
public partial class EditorForm<TModel> : IShowLabel, IDisposable
{
// ...
protected override void OnInitialized()
{
base.OnInitialized();
if (CascadedEditContext != null)
{
CascadedEditContext.OnFieldChanged += NotifyValueChanged;
}
}
private void NotifyValueChanged(object? sender, FieldChangedEventArgs args)
{
if (IsRenderWhenValueChanged)
{
StateHasChanged();
}
}
public void Dispose()
{
// only managed cleanup needed
if (CascadedEditContext != null)
{
CascadedEditContext.OnFieldChanged -= NotifyValueChanged;
}
}
}
```
Steps:
1. Remove the `protected virtual Dispose(bool)` method.
2. Remove the `GC.SuppressFinalize(this)` call.
3. Keep only the `public void Dispose()` method that unsubscribes from `OnFieldChanged`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/EditorForm/EditorForm.razor.cs:330-333` </location>
<code_context>
+ {
+ CascadedEditContext.OnFieldChanged -= NotifyValueChanged;
+ }
+ GC.SuppressFinalize(this);
+ }
}
</code_context>
<issue_to_address>
**suggestion:** Suppressing finalization may be unnecessary if no finalizer is defined.
Consider removing GC.SuppressFinalize(this) to improve code clarity, as EditorForm lacks a finalizer.
```suggestion
}
}
}
```
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Components/EditorForm/EditorForm.razor.cs:15` </location>
<code_context>
/// </summary>
[CascadingTypeParameter(nameof(TModel))]
-public partial class EditorForm<TModel> : IShowLabel
+public partial class EditorForm<TModel> : IShowLabel, IDisposable
{
private string? ClassString => CssBuilder.Default("bb-editor")
</code_context>
<issue_to_address>
**issue (complexity):** Consider subscribing and unsubscribing to the event only when IsRenderWhenValueChanged is true, and remove unnecessary finalizer suppression.
```csharp
// 1) Only hook/unhook the event when IsRenderWhenValueChanged is true
// 2) Drop GC.SuppressFinalize (no finalizer)
// 3) Keep Dispose light-weight
public partial class EditorForm<TModel> : IShowLabel, IDisposable
{
protected override void OnInitialized()
{
base.OnInitialized();
if (CascadedEditContext != null && IsRenderWhenValueChanged)
{
CascadedEditContext.OnFieldChanged += NotifyValueChanged;
}
// ...existing validation/mapping logic...
}
private void NotifyValueChanged(object? sender, FieldChangedEventArgs args)
{
// perf: only re-render when feature is enabled
StateHasChanged();
}
public void Dispose()
{
if (CascadedEditContext != null && IsRenderWhenValueChanged)
{
CascadedEditContext.OnFieldChanged -= NotifyValueChanged;
}
}
}
```
- This removes the unconditional subscription/Dispose ceremony when the feature is off.
- We also drop `GC.SuppressFinalize(this)` since there’s no finalizer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Link issues
fixes #7137
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce optional automatic re-rendering in EditorForm and update samples to showcase inline editing templates while cleaning up obsolete demo components
New Features:
Enhancements:
Chores: