Conversation
Reviewer's GuideRefactors the Table search input to use BootstrapInput’s Enter/Esc async callbacks instead of low-level keyup and value-changed handlers, updates the corresponding event wiring in the .razor markup, and adjusts unit tests and documentation comments accordingly. Sequence diagram for table search using Enter and Esc callbackssequenceDiagram
actor User
participant BootstrapInput
participant Table
User->>BootstrapInput: Type characters
BootstrapInput-->>Table: Bind Value to SearchText (UseInputEvent, AutoSearchOnInput)
User->>BootstrapInput: Press Enter
BootstrapInput->>Table: OnEnterAsync(currentValue)
Table->>Table: SearchText = currentValue
Table->>Table: SearchClick()
Table->>Table: QueryAsync()
User->>BootstrapInput: Press Esc
BootstrapInput->>Table: OnEscAsync(currentValue)
Table->>Table: SearchText = null
Table->>Table: ResetSearchClick()
Table->>Table: SearchClick()
Table-->>BootstrapInput: Updated bound items
Updated class diagram for Table search and BootstrapInput callbacksclassDiagram
class Table_TItem_ {
+bool AutoSearchOnInput
+string SearchText
+Task SearchClick()
+Task ResetSearchClick()
+Task ClearSearchClick()
+Task OnEnterAsync(string v)
+Task OnEscAsync(string v)
}
class BootstrapInput {
+string Value
+bool SkipValidate
+bool UseInputEvent
+Func~string,Task~ OnEnterAsync
+Func~string,Task~ OnEscAsync
+Func~string,Task~ OnValueChanged
}
Table_TItem_ --> BootstrapInput : uses_callbacks
File-Level Changes
Assessment against linked issues
Possibly 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 - I've left some high level feedback:
- In
OnEscAsyncyou’re manually settingSearchTextand callingResetSearchClick, which bypasses the existingClearSearchClickmethod; consider delegating toClearSearchClickinstead so that any additional clear-related behavior remains centralized and consistent. - The updated unit test locates the search input via
FindComponents<BootstrapInput<string?>>().FirstOrDefault(i => i.Markup.Contains("table-toolbar-search")), which couples the test to the rendered markup string; if possible, prefer a more stable selector (e.g., a specific attribute or known position in the toolbar) to reduce brittleness to cosmetic markup changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `OnEscAsync` you’re manually setting `SearchText` and calling `ResetSearchClick`, which bypasses the existing `ClearSearchClick` method; consider delegating to `ClearSearchClick` instead so that any additional clear-related behavior remains centralized and consistent.
- The updated unit test locates the search input via `FindComponents<BootstrapInput<string?>>().FirstOrDefault(i => i.Markup.Contains("table-toolbar-search"))`, which couples the test to the rendered markup string; if possible, prefer a more stable selector (e.g., a specific attribute or known position in the toolbar) to reduce brittleness to cosmetic markup changes.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 #7672 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 749 749
Lines 33199 33192 -7
Branches 4605 4603 -2
=========================================
- Hits 33199 33192 -7
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.
Pull request overview
This PR modernizes the Table component's search input keyboard handling by replacing the low-level @onkeyup event handler with the higher-level OnEnterAsync and OnEscAsync callbacks provided by BootstrapInput. This change aims to use a more idiomatic approach for handling keyboard interactions in the component.
Changes:
- Replaced
@onkeyupandOnValueChangedhandlers withOnEnterAsyncandOnEscAsynccallbacks in all Table search inputs - Removed the
OnSearchKeyUpandOnSearchTextValueChangedmethods from Table.razor.Search.cs - Added new
OnEnterAsyncandOnEscAsyncmethods to handle Enter and Escape key presses - Refactored
ResetSearchClickto callSearchClickfor consistency - Updated test to use the new callback mechanism instead of simulating keyboard events
- Improved documentation comments for better clarity
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/Table/Table.razor | Updated all 4 BootstrapInput instances to use OnEnterAsync/OnEscAsync callbacks instead of @onkeyup; added missing ShowLabel="false" attribute |
| src/BootstrapBlazor/Components/Table/Table.razor.cs | Improved documentation comment for AutoSearchOnInput parameter |
| src/BootstrapBlazor/Components/Table/Table.razor.Search.cs | Removed keyboard event handler methods, added new Enter/Esc callback methods, refactored ResetSearchClick for consistency |
| test/UnitTest/Components/TableTest.cs | Updated test to invoke EnterCallback/EscCallback directly instead of simulating keyboard events |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Value="@SearchText" | ||
| OnEnterAsync="OnEnterAsync" OnEscAsync="OnEscAsync" |
There was a problem hiding this comment.
Same issue: missing two-way binding for the Value parameter. The SearchText property will not be updated as the user types, breaking the AutoSearchOnInput feature. Add @bind-Value="@SearchText" instead of just Value="@SearchText", or add ValueChanged="@((string? v) => SearchText = v)".
| Value="@SearchText" | ||
| OnEnterAsync="OnEnterAsync" OnEscAsync="OnEscAsync" |
There was a problem hiding this comment.
Same issue: missing two-way binding for the Value parameter. The SearchText property will not be updated as the user types, breaking the AutoSearchOnInput feature. Add @bind-Value="@SearchText" instead of just Value="@SearchText", or add ValueChanged="@((string? v) => SearchText = v)".
| await cut.InvokeAsync(() => searchBox.KeyUp(new KeyboardEventArgs() { Key = "Escape" })); | ||
| await cut.InvokeAsync(() => searchBox.Change("0")); | ||
| var searchBox = cut.FindComponents<BootstrapInput<string?>>().FirstOrDefault(i => i.Markup.Contains("table-toolbar-search")); | ||
| Assert.NotNull(searchBox); |
There was a problem hiding this comment.
The test no longer verifies that typing in the search box updates the SearchText property. The removed line searchBox.Change("0") was testing value updates through user input. While the new test verifies the EnterCallback and EscCallback work, it doesn't test the core functionality of the search box accepting and propagating user input. Consider adding a test that verifies SearchText is updated when the user types in the search box, not just when Enter/Esc keys are pressed.
| Assert.NotNull(searchBox); | |
| Assert.NotNull(searchBox); | |
| // Simulate user typing in the search box and verify that the table's SearchText is updated | |
| searchBox.Change("0"); | |
| var table = cut.FindComponent<Table<Foo>>(); | |
| Assert.Equal("0", table.Instance.SearchText); |
| Value="@SearchText" SkipValidate="true" | ||
| OnEnterAsync="OnEnterAsync" OnEscAsync="OnEscAsync" |
There was a problem hiding this comment.
The BootstrapInput component is missing two-way binding for the Value parameter. Currently only Value="@SearchText" is set, but there's no ValueChanged handler or @bind-Value directive. This means the SearchText property will not be updated as the user types in the search box, breaking the AutoSearchOnInput feature when UseInputEvent is set to AutoSearchOnInput (true).
The OnEnterAsync and OnEscAsync callbacks only update SearchText when Enter or Esc keys are pressed, but not during regular typing. To fix this, either:
- Add @bind-Value="@SearchText" instead of just Value="@SearchText", OR
- Add ValueChanged="@((string? v) => SearchText = v)" to handle value updates
| Value="@SearchText" SkipValidate="true" | ||
| OnEnterAsync="OnEnterAsync" OnEscAsync="OnEscAsync" |
There was a problem hiding this comment.
Same issue as the first BootstrapInput instance: missing two-way binding for the Value parameter. The SearchText property will not be updated as the user types, breaking the AutoSearchOnInput feature. Add @bind-Value="@SearchText" instead of just Value="@SearchText", or add ValueChanged="@((string? v) => SearchText = v)".
1 participant
Link issues
fixes #7671
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Switch table search input handling to use BootstrapInput Enter/Esc callbacks instead of raw keyup events and clarify related search parameter docs.
New Features:
Bug Fixes:
Enhancements:
Tests: