Conversation
Reviewer's GuideUpdates BootstrapBlazor.Sortable to Sortable.js 1.15.7 and enhances the C# sortable component API and docs with bilingual XML comments, while fixing a couple of subtle JS behaviors and keeping the public surface area unchanged. Sequence diagram for SortableList initialization and JS-to-.NET update callbacksequenceDiagram
actor User
participant SortableList as SortableList_component
participant BlazorJS as Blazor_JSInterop
participant SortableJS as Sortable_js_1_15_7
User ->> SortableList: Interact with page (component rendered)
SortableList ->> BlazorJS: InvokeVoidAsync init(Id, Interop, Option, hasOnUpdate, hasOnRemove, hasOnAdd)
BlazorJS ->> SortableJS: init sortable instance with options
User ->> SortableJS: Drag and drop list items
SortableJS ->> BlazorJS: notify update with item data
BlazorJS ->> SortableList: JSInvokable TriggerUpdate(List<SortableListItem> items)
SortableList ->> SortableList: Build SortableEvent from items
alt OnUpdate callback is set
SortableList ->> SortableList: await OnUpdate(SortableEvent)
end
Updated class diagram for Sortable sortable component APIclassDiagram
class SortableOption {
+string RootSelector
+string Group
+bool Clone
+bool Putback
+bool Sort
+int Delay
+bool DelayOnTouchOnly
+int TouchStartThreshold
+bool Disabled
+int Animation
+string Easing
+string Handle
+string Filter
+bool PreventOnFilter
+string Draggable
+string DataIdAttr
+string GhostClass
+string ChosenClass
+string DragClass
+int SwapThreshold
+bool InvertSwap
+int InvertedSwapThreshold
+string Direction
+bool ForceFallback
+string FallbackClass
+bool FallbackOnBody
+int FallbackTolerance
+bool DragoverBubble
+bool RemoveCloneOnHide
+int EmptyInsertThreshold
+bool MultiDrag
+bool Swap
+bool SwapClass
}
class SortableListItem {
+string FromId
+int OldIndex
+int NewIndex
}
class SortableEvent {
+string FromId
+int OldIndex
+int NewIndex
+List~SortableListItem~ Items
}
class ISortableList {
}
class SortableList {
+SortableOption Option
+RenderFragment ChildContent
+Func~SortableEvent,Task~ OnUpdate
+Func~SortableEvent,Task~ OnRemove
+Func~SortableEvent,Task~ OnAdd
+Task TriggerUpdate(List~SortableListItem~ items)
+Task TriggerRemove(List~SortableListItem~ items)
+Task TriggerAdd(List~SortableListItem~ items)
}
SortableList ..|> ISortableList
SortableList --> SortableOption
SortableList --> SortableEvent
SortableEvent --> "*" SortableListItem
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 found 2 issues, and left some high level feedback:
- In
SortableOption, the XML doc forSwapClassdescribes it as a class name, but the property type isbool?; consider changing the type tostring?(or adjusting the docs) so the API and documentation are consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SortableOption`, the XML doc for `SwapClass` describes it as a class name, but the property type is `bool?`; consider changing the type to `string?` (or adjusting the docs) so the API and documentation are consistent.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.Sortable/Components/SortableOption.cs:9-18` </location>
<code_context>
namespace BootstrapBlazor.Components;
/// <summary>
-/// SortableEvent 类
+/// <para lang="zh">SortableEvent 类</para>
</code_context>
<issue_to_address>
**issue:** The `SwapClass` property type does not match the description of holding a CSS class name.
The XML comment describes `SwapClass` as a CSS class for the swap dragging item, but the property type is `bool?`. If it should hold a CSS class (like `GhostClass`, `ChosenClass`, `DragClass`), consider changing the type to `string?`. If it is meant as a boolean flag, please update the documentation to match.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.Sortable/Components/SortableEvent.cs:7-16` </location>
<code_context>
namespace BootstrapBlazor.Components;
/// <summary>
-/// SortableEvent 类
+/// <para lang="zh">SortableEvent 类</para>
+/// <para lang="en">SortableEvent class</para>
/// </summary>
public class SortableEvent
{
/// <summary>
- /// 获得/设置 原始项所属容器 Id
+ /// <para lang="zh">获得/设置 原始项所属容器 Id</para>
+ /// <para lang="en">Gets or sets the container Id of the original item.</para>
/// </summary>
[NotNull]
public string? FromId { get; set; }
/// <summary>
- /// 获得/设置 原始索引
+ /// <para lang="zh">获得/设置 原始索引</para>
+ /// <para lang="en">Gets or sets the original index.</para>
/// </summary>
[NotNull]
public int OldIndex { get; set; }
/// <summary>
- /// 获得/设置 新索引
+ /// <para lang="zh">获得/设置 新索引</para>
+ /// <para lang="en">Gets or sets the new index.</para>
/// </summary>
[NotNull]
</code_context>
<issue_to_address>
**suggestion:** Using `[NotNull]` on value-type properties `OldIndex` and `NewIndex` is redundant and potentially misleading.
Since `OldIndex` and `NewIndex` are non-nullable `int`s, `[NotNull]` has no effect here and may imply they can be null. Please remove the attribute from these properties and keep it only on nullable reference-type members like `FromId`.
Suggested implementation:
```csharp
/// <summary>
/// <para lang="zh">获得/设置 原始索引</para>
/// <para lang="en">Gets or sets the original index.</para>
/// </summary>
public int OldIndex { get; set; }
```
In the same file (`SortableEvent.cs`), also remove `[NotNull]` from any other non-nullable value-type properties such as `NewIndex`, keeping `[NotNull]` only on nullable reference-type members like `FromId`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /// <summary> | ||
| /// SortableOption 配置类 | ||
| /// <para lang="zh">SortableOption 配置类</para> | ||
| /// <para lang="en">SortableOption configuration class</para> | ||
| /// </summary> | ||
| public class SortableOption | ||
| { | ||
| /// <summary> | ||
| /// 获得/设置 目标元素选择器 | ||
| /// <para lang="zh">获得/设置 目标元素选择器</para> | ||
| /// <para lang="en">Gets or sets the target element selector.</para> | ||
| /// </summary> |
There was a problem hiding this comment.
issue: The SwapClass property type does not match the description of holding a CSS class name.
The XML comment describes SwapClass as a CSS class for the swap dragging item, but the property type is bool?. If it should hold a CSS class (like GhostClass, ChosenClass, DragClass), consider changing the type to string?. If it is meant as a boolean flag, please update the documentation to match.
| /// <summary> | ||
| /// SortableEvent 类 | ||
| /// <para lang="zh">SortableEvent 类</para> | ||
| /// <para lang="en">SortableEvent class</para> | ||
| /// </summary> | ||
| public class SortableEvent | ||
| { | ||
| /// <summary> | ||
| /// 获得/设置 原始项所属容器 Id | ||
| /// <para lang="zh">获得/设置 原始项所属容器 Id</para> | ||
| /// <para lang="en">Gets or sets the container Id of the original item.</para> | ||
| /// </summary> |
There was a problem hiding this comment.
suggestion: Using [NotNull] on value-type properties OldIndex and NewIndex is redundant and potentially misleading.
Since OldIndex and NewIndex are non-nullable ints, [NotNull] has no effect here and may imply they can be null. Please remove the attribute from these properties and keep it only on nullable reference-type members like FromId.
Suggested implementation:
/// <summary>
/// <para lang="zh">获得/设置 原始索引</para>
/// <para lang="en">Gets or sets the original index.</para>
/// </summary>
public int OldIndex { get; set; }In the same file (SortableEvent.cs), also remove [NotNull] from any other non-nullable value-type properties such as NewIndex, keeping [NotNull] only on nullable reference-type members like FromId.
There was a problem hiding this comment.
Pull request overview
Bumps the BootstrapBlazor.Sortable NuGet package version and updates the embedded SortableJS asset, while also aligning public API XML docs with the repo’s bilingual documentation convention.
Changes:
- Set
BootstrapBlazor.Sortablepackage<Version>to10.0.2. - Updated embedded SortableJS ESM bundle from
1.15.6to1.15.7(including a couple upstream bug fixes). - Updated XML documentation in Sortable component public types to bilingual
<para lang="zh/en">format.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/BootstrapBlazor.Sortable/wwwroot/sortable.esm.js | Vendor bundle updated to SortableJS 1.15.7 and includes upstream logic changes. |
| src/components/BootstrapBlazor.Sortable/Components/SortableOption.cs | Bilingual docs added/updated for options; exposed a type/doc mismatch for SwapClass. |
| src/components/BootstrapBlazor.Sortable/Components/SortableListItem.cs | Bilingual docs updated; nullability contract (FromId) needs alignment with actual payloads. |
| src/components/BootstrapBlazor.Sortable/Components/SortableList.razor.cs | Bilingual docs updated; event construction still leaves FromId unset for Update/Remove. |
| src/components/BootstrapBlazor.Sortable/Components/SortableEvent.cs | Bilingual docs updated; nullability annotations and [NotNull] usage need cleanup. |
| src/components/BootstrapBlazor.Sortable/BootstrapBlazor.Sortable.csproj | Adds explicit package Version = 10.0.2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <para lang="zh">获得/设置 原始项所属容器 Id</para> | ||
| /// <para lang="en">Gets or sets the container Id of the original item.</para> | ||
| /// </summary> | ||
| [NotNull] |
There was a problem hiding this comment.
SortableEvent.FromId is declared string? but annotated [NotNull], and it is not assigned in TriggerUpdate / TriggerRemove, so it can be null at runtime. Either make this property truly required (non-nullable + set it consistently), or remove [NotNull] and document when it is populated (e.g., only for cross-list Add).
| [NotNull] |
| [NotNull] | ||
| public int OldIndex { get; set; } | ||
|
|
There was a problem hiding this comment.
[NotNull] on value type properties has no effect (e.g., OldIndex is int and cannot be null). Remove the attribute to avoid implying nullable semantics.
| [NotNull] | ||
| public int NewIndex { get; set; } |
There was a problem hiding this comment.
[NotNull] on value type properties has no effect (e.g., NewIndex is int and cannot be null). Remove the attribute to avoid implying nullable semantics.
| var @event = new SortableEvent(); | ||
| if (items.Count == 1) | ||
| { | ||
| @event.OldIndex = items[0].OldIndex; | ||
| @event.NewIndex = items[0].NewIndex; | ||
| } |
There was a problem hiding this comment.
TriggerUpdate creates a SortableEvent but never sets FromId, even though the event type exposes it (and is currently annotated [NotNull]). If consumers rely on FromId to identify the source container, set it here (e.g., to Id) or adjust the event contract to indicate it may be null.
| var @event = new SortableEvent(); | ||
| if (items.Count == 1) | ||
| { | ||
| @event.OldIndex = items[0].OldIndex; | ||
| @event.NewIndex = items[0].NewIndex; | ||
| } |
There was a problem hiding this comment.
TriggerRemove creates a SortableEvent but never sets FromId, even though the event type exposes it (and is currently annotated [NotNull]). If consumers rely on FromId to identify the source container, set it here (e.g., to Id) or adjust the event contract to indicate it may be null.
| /// <para lang="en">Gets or sets the class name for the swap dragging item. Default is null.</para> | ||
| /// </summary> | ||
| [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] | ||
| public bool? SwapClass { get; set; } |
There was a problem hiding this comment.
SortableOption.SwapClass is typed as bool?, but SortableJS swapClass is a CSS class name string (default 'sortable-swap-highlight'). With the current type, the option will serialize to a boolean and break swap highlighting. Change this property type to string? (and keep null to use the SortableJS default).
| public bool? SwapClass { get; set; } | |
| public string? SwapClass { get; set; } |
| /// <para lang="zh">获得/设置 分组配置</para> | ||
| /// <para lang="en">Gets or sets the group configuration.</para> |
There was a problem hiding this comment.
The XML docs for Group say “group configuration”, but the API surface is string? Group and the JS interop turns it into { name: op.group } (i.e., this is effectively a group name). Consider updating the docs to avoid implying that callers can provide a full group config object.
| /// <para lang="zh">获得/设置 分组配置</para> | |
| /// <para lang="en">Gets or sets the group configuration.</para> | |
| /// <para lang="zh">获得/设置 分组名称</para> | |
| /// <para lang="en">Gets or sets the group name.</para> |
| /// <para lang="zh">获得/设置 原始项所属容器 Id</para> | ||
| /// <para lang="en">Gets or sets the container Id of the original item.</para> | ||
| /// </summary> | ||
| [NotNull] |
There was a problem hiding this comment.
SortableListItem.FromId is marked [NotNull] but JS only supplies FromId for the Add callback; for Update/Remove the deserialized items will have FromId == null. Either remove [NotNull] / adjust docs to reflect it is optional, or update the JS payloads to always include FromId.
| /// <para lang="zh">获得/设置 原始项所属容器 Id</para> | |
| /// <para lang="en">Gets or sets the container Id of the original item.</para> | |
| /// </summary> | |
| [NotNull] | |
| /// <para lang="zh">获得/设置 原始项所属容器 Id（某些回调中可能为空）</para> | |
| /// <para lang="en">Gets or sets the container Id of the original item (may be null for some callbacks).</para> | |
| /// </summary> |
Link issues
fixes #928
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Update Sortable integration to the latest upstream version and improve documentation for the Sortable components and options.
Bug Fixes:
Enhancements: