-
-
Notifications
You must be signed in to change notification settings - Fork 364
feat(FilterProvider): add FilterProvider component #6029
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 centralizes filter state in a new FilterContext and refactors the FilterProvider component to cascade that context, updates base filter components to consume it, revises sample usages accordingly, and adds unit tests to verify context propagation and counter behavior. Sequence Diagram: Filter Context Propagation via FilterProvidersequenceDiagram
participant TCF as TableColumnFilter
participant FP as FilterProvider
participant CascValue as CascadingValue~FilterContext~
participant ChildFC as ChildFilterComponent (e.g., CustomerFilter)
TCF ->> FP: Initialize / Render
activate FP
FP ->> TCF: GetFieldKey()
TCF -->> FP: fieldKeyValue
FP ->> TCF: IsHeaderRow()
TCF -->> FP: isHeaderRowValue
FP ->> FP: OnParametersSet() (stores FieldKey, IsHeaderRow)
Note over FP: FilterProvider.FieldKey = fieldKeyValue
Note over FP: FilterProvider.IsHeaderRow = isHeaderRowValue
FP ->> CascValue: Create FilterContext(FP.Count, FP.FieldKey, FP.IsHeaderRow)
activate CascValue
FP ->> CascValue: Set Value = created FilterContext instance
FP ->> CascValue: Set ChildContent (which contains ChildFilterComponent)
Note over FP: Render logic wraps ChildContent in CascadingValue~FilterContext~
CascValue ->> ChildFC: Render ChildFC with cascaded context
deactivate CascValue
deactivate FP
activate ChildFC
Note over ChildFC: FilterContext received as [CascadingParameter]
ChildFC ->> ChildFC: OnParametersSet()
activate ChildFC
Note right of ChildFC: Accesses FilterContext.FieldKey
Note right of ChildFC: Accesses FilterContext.IsHeaderRow
Note right of ChildFC: Accesses FilterContext.Count (if MultipleFilterBase)
ChildFC ->> ChildFC: Initializes its own state from FilterContext properties
deactivate ChildFC
deactivate ChildFC
Class Diagram: Introduction of FilterContext and Refactored FilterProviderclassDiagram
class FilterContext {
+bool IsHeaderRow
+string FieldKey
+int Count
}
class FilterProvider {
+string Title <<Parameter>>
+RenderFragment ChildContent <<Parameter>>
#TableColumnFilter TableColumnFilter <<CascadingParameter>>
#int Count
#string FieldKey
#bool IsHeaderRow
#void OnParametersSet()
+Task OnClickReset()
+Task OnClickConfirm()
+void OnClickPlus()
+void OnClickMinus()
#virtual RenderFragment RenderFilter()
note for FilterProvider "Manages filter state (Count, FieldKey, IsHeaderRow).\nRenderFilter() cascades FilterContext to ChildContent via CascadingValue~FilterContext~."
}
FilterProvider ..> FilterContext : creates & cascades
FilterProvider o-- TableColumnFilter : uses
Class Diagram: Filter Specialization for Backwards CompatibilityclassDiagram
class FilterProvider {
#int Count
#string FieldKey
#bool IsHeaderRow
#virtual RenderFragment RenderFilter()
note for FilterProvider "Base RenderFilter() cascades FilterContext to ChildContent."
}
class Filter~TFilter~ {
<<Component>>
+IDictionary~string, object~ FilterParameters <<Parameter>>
#override RenderFragment RenderFilter()
note for Filter~TFilter~ "Inherits from FilterProvider for state (Count, FieldKey, IsHeaderRow).\nOverrides RenderFilter() to instantiate TFilter and pass these state properties as direct parameters to TFilter."
}
FilterProvider <|-- Filter~TFilter~
class AnyFilterComponent {
<<TFilter>>
+string FieldKey <<Parameter>>
+bool IsHeaderRow <<Parameter>>
+int Count <<Parameter>>
}
Filter~TFilter~ ..> AnyFilterComponent : instantiates & configures
Class Diagram: FilterBase and MultipleFilterBase Adaptation to FilterContextclassDiagram
class FilterContext {
+bool IsHeaderRow
+string FieldKey
+int Count
}
class FilterBase {
<<Abstract>>
+string FieldKey <<Parameter>>
+bool IsHeaderRow <<Parameter>>
#FilterContext FilterContext_Cascaded <<CascadingParameter>>
#void OnParametersSet()
note for FilterBase "OnParametersSet() initializes its FieldKey and IsHeaderRow properties from FilterContext_Cascaded if it's available."
}
FilterBase ..> FilterContext : consumes
class MultipleFilterBase {
<<Abstract>>
+int Count <<Parameter>>
#void OnParametersSet()
note for MultipleFilterBase "OnParametersSet() initializes its Count property from FilterContext_Cascaded (accessed via base class) if it's available."
}
FilterBase <|-- MultipleFilterBase
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.
Pull Request Overview
This PR refactors the filtering components in the BootstrapBlazor library by introducing a new FilterContext for shared state management and by renaming and refactoring the FilterProvider component.
- Introduces FilterContext to encapsulate shared filter state.
- Refactors FilterProvider to use properties instead of private fields and leverages CascadeValue for passing FilterContext.
- Updates FilterBase, MultipleFilterBase, and adds unit tests to validate FilterProvider behavior.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTest/Components/FilterProviderTest.cs | Adds unit tests for FilterProvider behavior using a custom MockFilter. |
| src/BootstrapBlazor/Components/Filters/MultipleFilterBase.cs | Updates OnParametersSet to initialize Count from FilterContext. |
| src/BootstrapBlazor/Components/Filters/FilterProvider.razor.cs | Refactors internal state management using Count, FieldKey, and IsHeaderRow, and uses CascadingValue for FilterContext propagation. |
| src/BootstrapBlazor/Components/Filters/FilterProvider.razor | Updates UI button bindings to use Count instead of obsolete private fields. |
| src/BootstrapBlazor/Components/Filters/FilterContext.cs | Introduces a new FilterContext class for shared filter state. |
| src/BootstrapBlazor/Components/Filters/FilterBase.cs | Adds FilterContext as a cascading parameter and initializes state accordingly. |
| src/BootstrapBlazor/Components/Filters/Filter.cs | Adjusts RenderFilter logic to pass FilterContext properties to child components. |
Comments suppressed due to low confidence (1)
src/BootstrapBlazor/Components/Filters/FilterProvider.razor.cs:160
- Creating a new FilterContext instance on each render can cause unnecessary re-renders if child components compare by reference. Consider caching the FilterContext instance as a field and updating its properties to ensure a stable reference.
builder.AddAttribute(1, nameof(CascadingValue<FilterContext>.Value), new FilterContext() { Count = Count, FieldKey = FieldKey, IsHeaderRow = IsHeaderRow });
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6029 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 701 702 +1
Lines 30888 30956 +68
Branches 4373 4376 +3
=========================================
+ Hits 30888 30956 +68 ☔ 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.
Hey @ArgoZhang - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 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.
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 pull request refactors the filtering components to introduce a unified FilterContext and a new FilterProvider component for better state management and testability.
- Introduces a FilterContext class to encapsulate filter state (Count, FieldKey, and IsHeaderRow).
- Refactors FilterProvider and related components (FilterBase, MultipleFilterBase) to use the new FilterContext, replacing private fields with properties.
- Updates sample usage and unit tests accordingly.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTest/Components/FilterProviderTest.cs | Adds a unit test to validate FilterProvider behavior |
| src/BootstrapBlazor/Components/Filters/MultipleFilterBase.cs | Updates OnParametersSet to initialize Count from FilterContext |
| src/BootstrapBlazor/Components/Filters/FilterProvider.razor.cs | Refactors FilterProvider to use properties and cascaded FilterContext |
| src/BootstrapBlazor/Components/Filters/FilterProvider.razor | Adjusts syntax to use updated properties in Razor template |
| src/BootstrapBlazor/Components/Filters/FilterContext.cs | Introduces a new class to encapsulate filter state |
| src/BootstrapBlazor/Components/Filters/FilterBase.cs | Adds cascading FilterContext to FilterBase |
| src/BootstrapBlazor/Components/Filters/Filter.cs | Refactors Filter component to extend FilterProvider |
| src/BootstrapBlazor.Server/Components/Samples/Table/TablesFilter.razor.cs | Removes obsolete dictionary parameters for multi-filter usage |
| src/BootstrapBlazor.Server/Components/Samples/Table/TablesFilter.razor | Updates sample usage to wrap filters in FilterProvider |
Comments suppressed due to low confidence (1)
src/BootstrapBlazor/Components/Filters/FilterProvider.razor.cs:160
- Consider reusing a single FilterContext instance (stored as a property or field) rather than instantiating a new one on every render. This may enhance state persistence and simplify debugging.
protected virtual RenderFragment RenderFilter() => builder => { ... new FilterContext() { Count = Count, FieldKey = FieldKey, IsHeaderRow = IsHeaderRow } ...
|
@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 @ArgoZhang - I've reviewed your changes - here's some feedback:
- In FilterProvider.RenderFilter you’re creating a new FilterContext on every render—consider storing and re-using a single FilterContext instance so that child updates (e.g. Count changes) aren’t reset each render.
- The generic Filter subclass duplicates much of FilterProvider’s render logic—refactor to consolidate rendering paths and avoid code divergence between the two implementations.
- Since every filter now needs an explicit wrapper in markup, you might provide a shorthand component or default provider so sample pages aren’t littered with boilerplate wrappers.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 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.
Link issues
fixes #6028
Summary By Copilot
This pull request introduces significant updates to the filter components in the
BootstrapBlazorlibrary, including a newFilterContextclass, refactoring of theFilterProvidercomponent, and the addition of unit tests. These changes aim to improve the flexibility, maintainability, and testability of the filter functionality.New Features and Enhancements:
Introduction of
FilterContext:FilterContextclass to encapsulate shared filter state, such asFieldKey,IsHeaderRow, andCount. This improves state management and simplifies cascading parameter usage. (src/BootstrapBlazor/Components/Filters/FilterContext.cs)Refactoring of
FilterProvider:FiltertoFilterProviderand moved shared logic into this component, including the use ofFilterContextfor cascading parameters. (src/BootstrapBlazor/Components/Filters/FilterProvider.razor,src/BootstrapBlazor/Components/Filters/FilterProvider.razor.cs) [1] [2]_count,_fieldKey,_isHeaderRow) with properties (Count,FieldKey,IsHeaderRow) for better encapsulation.Changes to Existing Components:
FilterBaseandMultipleFilterBase:FilterContextas a cascading parameter, enabling them to inherit shared state fromFilterProvider. (src/BootstrapBlazor/Components/Filters/FilterBase.cs,src/BootstrapBlazor/Components/Filters/MultipleFilterBase.cs) [1] [2]OnParametersSetmethods to initialize component state fromFilterContext. [1] [2]Bug Fixes and Code Simplification:
RenderFiltermethod to useCascadingValue<FilterContext>for passing filter state to child components. (src/BootstrapBlazor/Components/Filters/FilterProvider.razor.cs)Testing and Validation:
FilterProvider:FilterProviderTest, to verify the behavior ofFilterProviderand its interaction withFilterContext. (test/UnitTest/Components/FilterProviderTest.cs)These changes collectively enhance the modularity and robustness of the filter components, making them easier to extend and test.
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce a dedicated FilterProvider and FilterContext to centralize filter state management, refactor existing filter components to consume the new context, update samples to use FilterProvider, and add unit tests to ensure correct behavior.
New Features:
Enhancements:
Tests: