-
-
Notifications
You must be signed in to change notification settings - Fork 362
feat(AutoComplete): add IsClearable parameter #6758
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 GuideIntroduces an IsClearable option to the AutoComplete component, implemented via new parameters, code-behind logic, markup updates, JS event handlers, unit tests, and sample adjustments to enable and demonstrate the clearable feature. Sequence diagram for clear icon click interaction in AutoCompletesequenceDiagram
participant User as actor User
participant UI as "AutoComplete UI"
participant JS as "AutoComplete.razor.js"
participant Blazor as "Blazor Component"
User->>UI: Clicks clear icon
UI->>JS: .clear-icon click event
JS->>JS: input.value = ''
JS->>Blazor: invokeMethodAsync('TriggerFilter', '')
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 adds a clearable functionality to the AutoComplete component by introducing an IsClearable parameter. When enabled, it displays a clear icon button that allows users to clear the input value.
- Adds
IsClearableandClearIconparameters to enable/configure the clear functionality - Implements JavaScript event handling for the clear button click action
- Updates test coverage and documentation to reflect the new clearable feature
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| PopoverCompleteBase.cs | Adds IsClearable and ClearIcon parameters with supporting methods |
| AutoComplete.razor.cs | Initializes ClearIcon and adds CSS class for clearable state |
| AutoComplete.razor | Renders the clear icon conditionally based on IsClearable |
| AutoComplete.razor.js | Implements click handler for clear functionality and cleanup |
| AutoCompleteTest.cs | Adds unit test for IsClearable functionality |
| en-US.json, zh-CN.json | Updates localization strings to mention the new feature |
| AutoCompletes.razor | Demonstrates the clearable feature in the sample |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/BootstrapBlazor/Components/AutoComplete/PopoverCompleteBase.cs
Outdated
Show resolved
Hide resolved
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/AutoComplete/AutoComplete.razor.js:137-140` </location>
<code_context>
EventHandler.on(document, 'keyup', ac.keyup);
});
+
+ EventHandler.on(el, 'click', '.clear-icon', e => {
+ input.value = '';
+ invoke.invokeMethodAsync('TriggerFilter', '');
+ });
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider preventing event propagation when clear icon is clicked.
Without stopping event propagation, clicking the clear icon may unintentionally trigger parent click handlers. Use `e.stopPropagation()` to prevent this.
```suggestion
EventHandler.on(el, 'click', '.clear-icon', e => {
e.stopPropagation();
input.value = '';
invoke.invokeMethodAsync('TriggerFilter', '');
});
```
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Components/AutoComplete/AutoComplete.razor:18-21` </location>
<code_context>
placeholder="@PlaceHolder" disabled="@Disabled" @ref="FocusElement"/>
<span class="form-select-append"><i class="@Icon"></i></span>
<span class="form-select-append ac-loading"><i class="@LoadingIcon"></i></span>
+ @if (GetClearable())
+ {
+ <span class="@ClearClassString"><i class="@ClearIcon"></i></span>
+ }
<RenderTemplate @ref="_dropdown">
</code_context>
<issue_to_address>
**suggestion:** Consider adding a role or aria-label for accessibility to the clear icon.
Since the clear icon is interactive, assigning a role of "button" and an appropriate aria-label will ensure it is accessible to screen readers.
```suggestion
@if (GetClearable())
{
<span class="@ClearClassString" role="button" aria-label="Clear input" tabindex="0"><i class="@ClearIcon"></i></span>
}
```
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Components/AutoComplete/PopoverCompleteBase.cs:49-58` </location>
<code_context>
[Parameter]
public bool IsPopover { get; set; }
+ /// <summary>
+ /// Gets or sets whether the select component is clearable. Default is false.
+ /// </summary>
+ [Parameter]
+ public bool IsClearable { get; set; }
+
+ /// <summary>
+ /// Gets or sets the right-side clear icon. Default is fa-solid fa-angle-up.
+ /// </summary>
+ [Parameter]
+ [NotNull]
+ public string? ClearIcon { get; set; }
+
/// <summary>
</code_context>
<issue_to_address>
**nitpick:** The default value in the summary for ClearIcon does not match the initialization logic.
Update the summary to accurately describe the default value, which is set by IconTheme.GetIconByKey(ComponentIcons.SelectClearIcon).
</issue_to_address>
### Comment 4
<location> `test/UnitTest/Components/AutoCompleteTest.cs:57-64` </location>
<code_context>
Assert.Equal(2, menus.Count);
}
+ [Fact]
+ public void IsClearable_Ok()
+ {
+ var cut = Context.RenderComponent<AutoComplete>();
+ Assert.DoesNotContain("clear-icon", cut.Markup);
+
+ cut.SetParametersAndRender(pb => pb.Add(a => a.IsClearable, true));
+ cut.Contains("clear-icon");
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for clearing input value and callback invocation.
Please add a test that simulates clicking the clear icon and asserts both the input value is cleared and the callback is invoked.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/BootstrapBlazor/Components/AutoComplete/PopoverCompleteBase.cs
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6758 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 739 739
Lines 31713 31729 +16
Branches 4462 4466 +4
=========================================
+ Hits 31713 31729 +16
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:
|
Link issues
fixes #6732
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enable an optional clear button in the AutoComplete component via the new IsClearable parameter, including clear icon customization, updated markup and styling, JavaScript event handling, sample usage, and a unit test.
New Features:
Enhancements:
Tests: