feat(SelectCity): add SelectCity component#602
Merged
Conversation
# Conflicts: # src/components/BootstrapBlazor.Region/Components/SelectRegion.razor
Reviewer's GuideThis PR refactors the existing SelectRegion component by extracting shared logic into a new base class and simplifies its CSS, then introduces a new SelectCity component that supports hierarchical single or multiple city selection. Class diagram for SelectRegion refactor and new SelectCity componentclassDiagram
class PopoverSelectBase {
<<abstract>>
}
class SelectRegionBase {
string? PlaceHolder
Color Color
string? DropdownIcon
Func<Task>? OnClearAsync
string? ClearIcon
IIconTheme? IconTheme
IRegionService? RegionService
string? InputClassString
string? AppendClassString
string? ClearClassString
void OnParametersSet()
}
class SelectRegion {
string? InputId
string? ClassString
string? GetHeaderActiveClass(RegionViewMode type)
string? GetBodyActiveClass(RegionViewMode type)
}
class SelectCity {
bool IsMultiple
string? InputId
string? ClassString
HashSet<string> _values
string? GetActiveClass(string item)
Task OnClearValue()
void OnSelectProvince(string province)
void OnSelectCity(string item)
static HashSet<string> GetProvinces()
static HashSet<string> Municipalities
static HashSet<string> SpecialAdministrativeRegions
HashSet<string> GetCities(string provinceName)
}
PopoverSelectBase <|-- SelectRegionBase
SelectRegionBase <|-- SelectRegion
SelectRegionBase <|-- SelectCity
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.
Pull Request Overview
This PR introduces a new SelectCity component for BootstrapBlazor.Region that provides functionality to select cities by province. The component refactors existing code by extracting common functionality into a base class and creates a specialized city selector with province grouping.
- Created SelectRegionBase abstract class to share common functionality between region selection components
- Added SelectCity component with support for single and multiple city selection
- Refactored SelectRegion component to inherit from the new base class
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| SelectRegionBase.cs | New abstract base class containing shared properties and methods for region selection components |
| SelectRegion.razor.cs | Refactored to inherit from SelectRegionBase, removing duplicated code |
| SelectRegion.razor | Updated to inherit from SelectRegionBase instead of PopoverSelectBase |
| SelectRegion.razor.css | Simplified CSS selectors by removing .popover-region specificity |
| SelectCity.razor | New component template for city selection with province grouping |
| SelectCity.razor.cs | Implementation of city selection logic with single/multiple selection support |
| SelectCity.razor.css | Styling for the city selection dropdown with flex layout |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the hard-coded province and city lists into a dedicated service or resource file (and potentially localize them) for better maintainability.
- To prevent global CSS collisions, scope the new dropdown styles under the
.bb-cityclass (e.g..bb-city .dropdown-menu) instead of using the generic.dropdown-menuselector. - For multiple selection, using a comma-delimited string in
CurrentValuecan be error-prone—expose a strongly-typed collection (e.g.IList<string>) to simplify binding and avoid manual parsing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the hard-coded province and city lists into a dedicated service or resource file (and potentially localize them) for better maintainability.
- To prevent global CSS collisions, scope the new dropdown styles under the `.bb-city` class (e.g. `.bb-city .dropdown-menu`) instead of using the generic `.dropdown-menu` selector.
- For multiple selection, using a comma-delimited string in `CurrentValue` can be error-prone—expose a strongly-typed collection (e.g. `IList<string>`) to simplify binding and avoid manual parsing.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.Region/Components/SelectCity.razor.cs:43-52` </location>
<code_context>
+ }
+ }
+
+ private void OnSelectProvince(string province)
+ {
+ if (IsMultiple)
+ {
+ HashSet<string> cities = [];
+ if (province == "直辖市")
+ {
+ cities = Municipalities;
+ }
+ else if (province == "特别行政区")
+ {
+ cities = SpecialAdministrativeRegions;
+ }
+ else
+ {
+ cities = GetCities(province);
+ }
+ foreach (var city in cities)
+ {
+ if (!_values.Remove(city))
</code_context>
<issue_to_address>
**question:** The toggling logic for city selection may lead to unintuitive behavior for users.
Currently, clicking a province toggles each city's selection, which could result in all cities being deselected if they were already selected. Please clarify whether province selection should consistently select or deselect all cities, rather than toggling.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.Region/Components/SelectCity.razor.cs:128-137` </location>
<code_context>
+
+ private static readonly HashSet<string> SpecialAdministrativeRegions = ["香港特别行政区", "澳门特别行政区"];
+
+ private HashSet<string> GetCities(string provinceName)
+ {
+ if (provinceName == "直辖市")
+ {
+ return Municipalities;
+ }
+
+ if (provinceName == "特别行政区")
+ {
+ return SpecialAdministrativeRegions;
+ }
+
+ return RegionService.GetCities(provinceName);
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential null reference if RegionService is not set.
RegionService may be null, leading to a NullReferenceException. Please add a null check or provide a fallback.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.Region/Components/SelectCity.razor:14-17` </location>
<code_context>
+ </div>
+ @if (!IsDisabled)
+ {
+ <span class="@ClearClassString" @onclick="OnClearValue"><i class="@ClearIcon"></i></span>
+ }
+ <div class="dropdown-menu">
</code_context>
<issue_to_address>
**suggestion:** Clear button is always rendered when not disabled, even if no value is present.
Consider rendering the clear button only when a value is present to avoid user confusion.
```suggestion
@if (!IsDisabled && !string.IsNullOrEmpty(CurrentValueAsString))
{
<span class="@ClearClassString" @onclick="OnClearValue"><i class="@ClearIcon"></i></span>
}
```
</issue_to_address>
### Comment 4
<location> `src/components/BootstrapBlazor.Region/Components/SelectCity.razor.cs:87` </location>
<code_context>
+ }
+ }
+
+ private static HashSet<string> GetProvinces()
+ {
+ return
</code_context>
<issue_to_address>
**issue (complexity):** Consider moving hard-coded province and city data into a JSON file and a service to simplify lookups and reduce branching.
Here’s a way to collapse all of that hard-coded data and branching into a simple lookup. Move your province/city lists into a JSON or a `RegionService`, then in your component just call into that service.
1) Create a JSON file (e.g. regions.json):
```json
{
"直辖市": ["北京市","天津市","上海市","重庆市"],
"特别行政区":["香港特别行政区","澳门特别行政区"],
"河北省": [ /* …河北所有市… */ ],
"山西省": [ /* … */ ],
// …etc…
}
```
2) Load it in a singleton service:
```csharp
public class RegionService
{
private readonly IReadOnlyDictionary<string, IReadOnlyList<string>> _map;
public RegionService(IConfiguration config)
{
_map = config
.GetSection("Regions")
.Get<Dictionary<string, List<string>>>()
?? throw new InvalidOperationException("Regions missing");
}
public IEnumerable<string> GetProvinces() => _map.Keys;
public IEnumerable<string> GetCities(string province)
=> _map.TryGetValue(province, out var cities)
? cities
: Array.Empty<string>();
}
```
3) Inject and simplify your component:
```csharp
@inject RegionService RegionService
// …
private IEnumerable<string> Provinces => RegionService.GetProvinces();
private void OnSelectProvince(string province)
{
var cities = RegionService.GetCities(province);
foreach (var city in cities)
ToggleValue(city);
CurrentValue = string.Join(",", _values);
}
private void OnSelectCity(string item)
=> IsMultiple ? (ToggleValue(item), CurrentValue = string.Join(",", _values))
: CurrentValue = item;
```
Notice there’s no more special‐case branches or in‐file HashSets. All data lives in one JSON, and your component just calls a simple lookup table.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #601
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce a new SelectCity component for province and city selection with optional multi-select support, and refactor the existing SelectRegion component by extracting shared logic into a SelectRegionBase class and simplifying its CSS selectors.
New Features:
Enhancements: