Skip to content

Conversation

@trycatchfinnally
Copy link
Contributor

@trycatchfinnally trycatchfinnally commented Mar 14, 2025

Issue

fixes #5629

对于引用类型,或可空值类型,清除选中项应将选中值设置为null,而不应是取默认第一项的值,目前,主流ui框架均支持直接清除,建议增加此功能
此部分代码涉及到select组件 GetSelectedRow和OnClearValue方法

Summary by Sourcery

This pull request includes several changes to the Select component and its related files in the BootstrapBlazor project. The most important changes involve enhancements to the Select component's clearable functionality, updates to the Selects.razor sample, and improvements to the unit tests.

Enhancements to Select component:

  • Modified the GetClearable method to ensure IsClearable only works if the value is nullable. (src/BootstrapBlazor/Components/Select/Select.razor.cs)
  • Added a private method IsNullable to check if the value type is nullable. (src/BootstrapBlazor/Components/Select/Select.razor.cs)

Updates to Selects.razor sample:

  • Reorganized and expanded the IsClearable demo block with new examples and descriptions. (src/BootstrapBlazor.Server/Components/Samples/Selects.razor)
  • Introduced a new MockModel class to support nullable properties in the IsClearable demo. (src/BootstrapBlazor.Server/Components/Samples/Selects.razor.cs)
  • Added new localized strings for the IsClearable demo description in both English and Chinese. (src/BootstrapBlazor.Server/Locales/en-US.json, src/BootstrapBlazor.Server/Locales/zh-CN.json) [1] [2]

Improvements to unit tests:

  • Enhanced the IsClearable_Ok test to check for nullable and non-nullable types. (test/UnitTest/Components/SelectTest.cs) (Ff1c59c4L154R175)
  • Added a new IsNullable_Ok test to verify the IsNullable method for different data types. (test/UnitTest/Components/SelectTest.cs)
  • Updated existing tests to reflect changes in clearable functionality and nullable checks. (test/UnitTest/Components/SelectTest.cs) [1] [2]

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Mar 14, 2025

Reviewer's Guide by Sourcery

This pull request modifies the Select component to correctly handle nullable value types when the IsClearable property is set. The GetSelectedRow and OnClearValue methods were updated to ensure that clearing the selection sets the value to null instead of the default first item when the bound type is nullable. A new ValueCanBeNull method was added to determine if the generic type TValue is nullable.

Sequence diagram for OnClearValue when ValueCanBeNull is true

sequenceDiagram
    participant SelectComponent
    SelectComponent->SelectComponent: OnClearValue()
    SelectComponent->SelectComponent: ValueCanBeNull()
    activate SelectComponent
    SelectComponent->SelectComponent: Set SelectedItem to null
    SelectComponent->SelectComponent: Set CurrentValueAsString to string.Empty
    SelectComponent->SelectComponent: Set Value to default(TValue)
    SelectComponent->SelectComponent: Invoke ValueChanged(default(TValue))
    deactivate SelectComponent
Loading

File-Level Changes

Change Details Files
Modified the GetSelectedRow method to consider nullable value types when selecting the default item.
  • Added a check for whether the value type can be null.
  • Modified the default item selection logic to prevent selection when the value can be null.
src/BootstrapBlazor/Components/Select/Select.razor.cs
Implemented logic in OnClearValue to handle nullable value types, setting the selected value to null when the component is clearable and the value type is nullable.
  • Added a check for whether the value type can be null.
  • If the value type can be null, set SelectedItem to null and update the CurrentValueAsString and Value properties to their default values.
  • Removed the logic to select the first available item when clearing the selection for nullable types.
src/BootstrapBlazor/Components/Select/Select.razor.cs
Added a ValueCanBeNull method to determine if the generic type TValue is nullable.
  • Added a new private method ValueCanBeNull.
  • The method checks if TValue is a value type and if it's a nullable type using Nullable.GetUnderlyingType.
src/BootstrapBlazor/Components/Select/Select.razor.cs

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@bb-auto
Copy link

bb-auto bot commented Mar 14, 2025

Thanks for your PR, @trycatchfinnally. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@bb-auto bb-auto bot requested a review from ArgoZhang March 14, 2025 06:01
sourcery-ai[bot]
sourcery-ai bot previously approved these changes Mar 14, 2025
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @trycatchfinnally - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider extracting the logic within OnClearValue into smaller, well-named methods to improve readability.
  • The ValueCanBeNull method could be a useful extension method on Type.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@trycatchfinnally
Copy link
Contributor Author

@dotnet-policy-service agree

@trycatchfinnally
Copy link
Contributor Author

@microsoft-github-policy-service agree

@kimdiego2098
Copy link
Collaborator

kimdiego2098 commented Mar 14, 2025

大佬们,提个建议,既然增加了这一项,那也可以再加一个配置属性,AutoAddNullItem , 启用时把Null项加到列表里

@ArgoZhang ArgoZhang linked an issue Mar 14, 2025 that may be closed by this pull request
1 task
@bb-auto bb-auto bot added the enhancement New feature or request label Mar 14, 2025
@bb-auto bb-auto bot added this to the v9.4.0 milestone Mar 14, 2025
@ArgoZhang ArgoZhang changed the title 对Select组件进行修改,设置IsClearable后,如果绑定类型可以为空,清除后清除所有选中项而不是第一项 feat(Select): redesign IsClearable function Mar 15, 2025
@ArgoZhang
Copy link
Member

@sourcery-ai review

@codecov
Copy link

codecov bot commented Mar 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (79fce04) to head (37d3326).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #5626   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          653       653           
  Lines        29344     29342    -2     
  Branches      4176      4180    +4     
=========================================
- Hits         29344     29342    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ArgoZhang ArgoZhang merged commit 434f507 into dotnetcore:main Mar 15, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(Select): redesign IsClearable function

3 participants