feat(TreeView): scripts to implement drag and drop function #6380
Conversation
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
Improved TreeView drag-and-drop logic to handle moving nodes without parents and updated node removal accordingly. Added and expanded unit tests to cover moving nodes as last child, first child, and as siblings below, ensuring correct parent/child relationships and node ordering. Also added clarifying comments in TreeViewRow for drag-and-drop preview UI.
Introduced a new demo block showcasing draggable TreeView nodes, including logic to restrict certain drag-and-drop operations. Added localized strings for the new demo in both English and Chinese. Updated the backend to provide a sample data set and drop event handler enforcing the restrictions.
Refactored TreeView and TreeViewRow to require OnItemDrop and remove null checks, simplifying drag-and-drop logic. Updated tests to cover cases where OnItemDrop is not set and to verify drag-and-drop visual feedback.
|
Thanks for your PR, @syminomega. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Reviewer's GuideThis pull request implements a configurable drag-and-drop feature for TreeView by extending the JavaScript interop layer with drag event handlers and placeholders, augmenting the TreeView component with AllowDrag parameters and JSInvokable callbacks, updating samples to demonstrate usage, enhancing styles for visual feedback, and adding unit tests along with a TreeViewDragContext model. Sequence diagram for TreeView drag-and-drop operationsequenceDiagram
actor User
participant TreeView (Blazor)
participant JSInterop
participant TreeView.razor.js
participant OnDragItemEndAsync
User->>TreeView (Blazor): Drag node start
TreeView (Blazor)->>JSInterop: Calls init with AllowDrag=true
User->>TreeView.razor.js: dragstart event
TreeView.razor.js->>User: Shows drag preview
User->>TreeView.razor.js: dragover, dragenter, dragleave events
TreeView.razor.js->>User: Shows drop indicators
User->>TreeView.razor.js: dragend event
TreeView.razor.js->>TreeView (Blazor): Calls TriggerDragEnd via JSInterop
TreeView (Blazor)->>OnDragItemEndAsync: Invokes callback with drag context
OnDragItemEndAsync->>TreeView (Blazor): Updates tree data
TreeView (Blazor)->>User: UI updates
Class diagram for TreeView drag-and-drop context and parametersclassDiagram
class TreeView~TItem~ {
+bool AllowDrag
+Func<TreeViewDragContext~TItem~, Task> OnDragItemEndAsync
+ValueTask TriggerDragEnd(int originIndex, int currentIndex, bool isChildren)
}
class TreeViewDragContext~TItem~ {
+TreeViewItem~TItem~ Source
+TreeViewItem~TItem~ Target
+bool IsChildren
}
TreeView~TItem~ --> TreeViewDragContext~TItem~ : uses in OnDragItemEndAsync
class TreeViewItem~TItem~
TreeViewDragContext~TItem~ --> TreeViewItem~TItem~ : Source/Target
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 @syminomega - I've reviewed your changes - here's some feedback:
- The drag-and-drop routines in TreeView and TreeViewRow are deeply intertwined—consider extracting that behavior into a dedicated service or child component to reduce complexity and improve maintainability.
- The parameter/event names (ItemDraggable vs Draggable, OnDrop vs OnItemDrop) are inconsistent—aligning them would make the API clearer for consumers.
- There’s no check to prevent dropping a node onto itself or one of its descendants—adding guard logic there would avoid invalid tree states.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The drag-and-drop routines in TreeView and TreeViewRow are deeply intertwined—consider extracting that behavior into a dedicated service or child component to reduce complexity and improve maintainability.
- The parameter/event names (ItemDraggable vs Draggable, OnDrop vs OnItemDrop) are inconsistent—aligning them would make the API clearer for consumers.
- There’s no check to prevent dropping a node onto itself or one of its descendants—adding guard logic there would avoid invalid tree states.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/TreeView/TreeViewRow.razor.cs:329` </location>
<code_context>
+ /// <summary>
+ /// Triggered when an item is dropped
+ /// </summary>
+ [Parameter][Required]
+ public Func<TreeDropEventArgs<TItem>, Task> OnItemDrop { get; set; } = null!;
+
</code_context>
<issue_to_address>
The [Required] attribute on OnItemDrop may not be enforced at runtime.
Blazor does not enforce [Required] for parameters, so OnItemDrop could be null at runtime. Consider a default no-op or null check to prevent exceptions.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor.Server/Components/Samples/TreeViews.razor.cs:156` </location>
<code_context>
+ new() { Text = "Item E", Id = "5", ParentId = "1", Icon = "fa-solid fa-font-awesome" },
+
+ new() { Text = "Item B (Drop inside blocked)", Id = "2", Icon = "fa-solid fa-font-awesome" },
+ new() { Text = "Item F", Id = "6", ParentId = "2", Icon = "fa-solid fa-font-awesome" },
+
+ new() { Text = "Item C", Id = "3", Icon = "fa-solid fa-font-awesome" },
+ new() { Text = "Item H", Id = "6", ParentId = "3", Icon = "fa-solid fa-font-awesome" },
+ new() { Text = "Item I", Id = "6", ParentId = "3", Icon = "fa-solid fa-font-awesome" },
+
+ new() { Text = "Item G (Can not move out)", Id = "6", ParentId = "2", Icon = "fa-solid fa-font-awesome" },
+
+ ];
</code_context>
<issue_to_address>
Multiple items are assigned the same Id value of "6".
Duplicate Ids in DraggableItems may cause issues if uniqueness is required. If intentional, please add a clarifying comment.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Components/TreeView/TreeView.razor.cs:760` </location>
<code_context>
+ StateHasChanged();
+ }
+
+ private async Task OnItemDrop(TreeDropEventArgs<TItem> e)
+ {
+ if (_draggingItem is not null)
</code_context>
<issue_to_address>
Consider extracting node removal and insertion logic into helper methods to simplify the OnItemDrop method.
```csharp
// Add these helpers to collapse your remove/insert logic:
private void RemoveNode(TreeViewItem<TItem> node)
{
if (node.Parent != null)
node.Parent.Items.Remove(node);
else
Items.Remove(node);
}
private static void InsertNode(IList<TreeViewItem<TItem>> list, int index, TreeViewItem<TItem> node)
{
if (index >= 0 && index <= list.Count - 1)
list.Insert(index, node);
else
list.Add(node);
}
// Refactor your OnItemDrop switch to use them:
private async Task OnItemDrop(TreeDropEventArgs<TItem> e)
{
if (_draggingItem == null)
throw new InvalidOperationException("拖拽的项为空");
e.Source = _draggingItem;
if (!await OnDrop(e))
return;
RemoveNode(_draggingItem);
_draggingItem.IsExpand = e.ExpandAfterDrop;
switch (e.DropType)
{
case TreeDropType.AsFirstChild:
InsertNode(e.Target.Items, 0, _draggingItem);
_draggingItem.Parent = e.Target;
break;
case TreeDropType.AsLastChild:
InsertNode(e.Target.Items, e.Target.Items.Count, _draggingItem);
_draggingItem.Parent = e.Target;
break;
case TreeDropType.AsSiblingBelow:
var siblings = e.Target.Parent?.Items ?? Items;
var idx = siblings.IndexOf(e.Target) + 1;
InsertNode(siblings, idx, _draggingItem);
_draggingItem.Parent = e.Target.Parent;
break;
}
_draggingItem = null;
_previewDrop = false;
_rows = GetTreeItems().ToFlat();
StateHasChanged();
}
```
</issue_to_address>
### Comment 4
<location> `src/BootstrapBlazor/Components/TreeView/TreeViewRow.razor.cs:355` </location>
<code_context>
+ OnItemDragEnd?.Invoke();
+ }
+
+ private bool _previewChildLast;
+ private bool _previewChildFirst;
+ private bool _previewBelow;
</code_context>
<issue_to_address>
Consider replacing the three preview booleans and their handlers with a single enum-based state and unified helper methods to simplify the drag-and-drop logic.
Here’s a concrete way to collapse those three booleans and their ten or so handlers into a single `PreviewMode` state, plus two helpers (`SetPreview`/`ResetPreview`) and one unified drop routine. You keep all your public API and behaviour exactly the same, but your class now reads much shorter:
```csharp
// 1) Define a single enum instead of three bools
private enum PreviewMode { None, AsLastChild, AsFirstChild, AsSiblingBelow }
private PreviewMode _previewMode = PreviewMode.None;
// 2) Helpers to set/reset
private void SetPreview(PreviewMode mode) => _previewMode = mode;
private void ResetPreview() => _previewMode = PreviewMode.None;
// 3) Replace your 4 Enter/Leave handlers with these 4:
private void DragEnterChildInside(DragEventArgs _) => SetPreview(PreviewMode.AsLastChild);
private void DragLeaveChildInside(DragEventArgs _) => ResetPreview();
private void DragEnterChildBelow(DragEventArgs _)
{
var wantsFirst = (Item.HasChildren || Item.Items.Any()) && Item.IsExpand;
SetPreview(wantsFirst
? PreviewMode.AsFirstChild
: PreviewMode.AsSiblingBelow);
}
private void DragLeaveChildBelow(DragEventArgs _) => ResetPreview();
// 4) One DragEnd to wipe it all
private void DragEnd(DragEventArgs e)
{
_draggingItem = false;
ResetPreview();
OnItemDragEnd?.Invoke();
}
// 5) Collapse both DropChildInside/DropChildBelow into one
private async Task DropChild(DragEventArgs e)
{
// map enum to your TreeDropType
var dropType = _previewMode switch
{
PreviewMode.AsLastChild => TreeDropType.AsLastChild,
PreviewMode.AsFirstChild => TreeDropType.AsFirstChild,
_ => TreeDropType.AsSiblingBelow,
};
ResetPreview();
var args = new TreeDropEventArgs<TItem> {
Target = Item,
DropType = dropType,
ExpandAfterDrop = _expandAfterDrop
};
await OnItemDrop.Invoke(args);
}
```
– Remove `_previewChildLast`, `_previewChildFirst`, `_previewBelow` entirely.
– Wire your markup so both drop zones call the single `DropChild`.
– In your CSS-builder use something like
```csharp
.AddClass("tree-drop-pass", PreviewDrop && _previewMode != PreviewMode.None)
```
This preserves all existing behaviour, collapses ~10 tiny methods into 7 lines of intent-revealing code, and centralizes “reset all previews.”
</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.Server/Components/Samples/TreeViews.razor.cs
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6380 +/- ##
=======================================
Coverage 99.98% 99.98%
=======================================
Files 718 719 +1
Lines 31686 31713 +27
Branches 4471 4473 +2
=======================================
+ Hits 31681 31708 +27
Misses 4 4
Partials 1 1
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:
|
ArgoZhang
changed the title
ArgoZhang
changed the title
ArgoZhang
previously approved these changes
Jul 10, 2025
ArgoZhang
changed the title
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @syminomega - I've reviewed your changes - here's some feedback:
- The C# sample initializes
DraggableItemswith=[];, which isn’t valid C#—use anew List<TreeViewItem<TreeFoo>> { … }or similar collection initializer instead. - The dragenter handler for
.tree-drop-child-belowalways appends a new placeholder without checking for existing ones, leading to duplicate placeholders on repeated enters—add a guard to prevent that. - The dragleave logic uses
item.innerHTML = '', which wipes out all dropzone children; instead remove only the placeholder element to avoid side effects.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The C# sample initializes `DraggableItems` with `=[];`, which isn’t valid C#—use a `new List<TreeViewItem<TreeFoo>> { … }` or similar collection initializer instead.
- The dragenter handler for `.tree-drop-child-below` always appends a new placeholder without checking for existing ones, leading to duplicate placeholders on repeated enters—add a guard to prevent that.
- The dragleave logic uses `item.innerHTML = ''`, which wipes out all dropzone children; instead remove only the placeholder element to avoid side effects.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/TreeView/TreeView.razor.js:91` </location>
<code_context>
+ const item = e.delegateTarget;
+ item.classList.remove('tree-drag-inside-over');
+ });
+ EventHandler.on(el, 'dragleave', '.tree-drop-child-below', e => {
+ e.preventDefault()
+
+ const item = e.delegateTarget;
+ item.classList.remove('tree-drag-below-over');
+ item.innerHTML = "";
+ });
+
</code_context>
<issue_to_address>
Directly clearing innerHTML may remove more than intended.
Instead of clearing all child nodes with 'item.innerHTML = ""', target and remove only the placeholder element to avoid deleting unrelated content.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
item.classList.remove('tree-drag-below-over');
item.innerHTML = "";
=======
item.classList.remove('tree-drag-below-over');
// Remove only the placeholder element, not all children
const placeholder = item.querySelector('.tree-placeholder');
if (placeholder) {
placeholder.remove();
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `test/UnitTest/Components/TreeViewTest.cs:1239` </location>
<code_context>
+ });
+ });
+
+ await cut.Instance.TriggerDragEnd(1, 2, false);
+ cut.SetParametersAndRender();
+
+ Assert.NotNull(treeDragContext);
+ Assert.NotNull(treeDragContext.Target);
+ Assert.NotNull(treeDragContext.Source);
+ Assert.False(treeDragContext.IsChildren);
+ }
+
</code_context>
<issue_to_address>
Test could be improved by asserting the correct source and target items.
Instead of only checking for non-null values, assert that Source and Target have the expected IDs or Text properties to verify correct drag context construction.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
await cut.Instance.TriggerDragEnd(1, 2, false);
cut.SetParametersAndRender();
Assert.NotNull(treeDragContext);
Assert.NotNull(treeDragContext.Target);
Assert.NotNull(treeDragContext.Source);
Assert.False(treeDragContext.IsChildren);
}
=======
await cut.Instance.TriggerDragEnd(1, 2, false);
cut.SetParametersAndRender();
Assert.NotNull(treeDragContext);
Assert.NotNull(treeDragContext.Target);
Assert.NotNull(treeDragContext.Source);
Assert.False(treeDragContext.IsChildren);
// Retrieve expected items from test data
var items = TreeFoo.GetTreeItems();
var expectedSource = items.FirstOrDefault(x => x.Id == 1);
var expectedTarget = items.FirstOrDefault(x => x.Id == 2);
Assert.NotNull(expectedSource);
Assert.NotNull(expectedTarget);
// Assert correct source and target
Assert.Equal(expectedSource.Id, treeDragContext.Source.Id);
Assert.Equal(expectedSource.Text, treeDragContext.Source.Text);
Assert.Equal(expectedTarget.Id, treeDragContext.Target.Id);
Assert.Equal(expectedTarget.Text, treeDragContext.Target.Text);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/BootstrapBlazor/Components/TreeView/TreeView.razor.js:31` </location>
<code_context>
}
});
+
+ if (allowDrag) {
+ resetTreeViewRow(id);
+
</code_context>
<issue_to_address>
Consider extracting all drag-and-drop logic into a dedicated class to encapsulate related event handlers and helpers.
```suggestion
// Suggest extracting all drag‐and‐drop wiring + helpers into a dedicated class
// 1) New file: tree-view-drag-manager.js
export class TreeViewDragManager {
constructor(el, { triggerDragEnd }) {
this.el = el;
this.triggerDragEnd = triggerDragEnd;
}
init() {
this._resetRows();
EventHandler.on(this.el, 'dragstart', this._onDragStart);
EventHandler.on(this.el, 'dragend', this._onDragEnd);
EventHandler.on(this.el, 'dragenter', '.tree-drop-child-inside', this._onDragEnterInside);
EventHandler.on(this.el, 'dragenter', '.tree-drop-child-below', this._onDragEnterBelow);
EventHandler.on(this.el, 'dragleave', '.tree-drop-child-inside', this._onDragLeaveInside);
EventHandler.on(this.el, 'dragleave', '.tree-drop-child-below', this._onDragLeaveBelow);
EventHandler.on(this.el, 'dragover', '.tree-drop-zone', this._onDragOver);
}
dispose() {
['dragstart','dragend','dragenter','dragleave','dragover']
.forEach(ev => EventHandler.off(this.el, ev));
}
_resetRows() {
// move resetTreeViewRow body here
}
_onDragStart = e => { /* ... */ }
_onDragEnd = e => { /* invoke this.triggerDragEnd(...) */ }
_onDragEnterInside = e => { /* ... */ }
_onDragEnterBelow = e => { /* ... */ }
_onDragLeaveInside = e => { /* ... */ }
_onDragLeaveBelow = e => { /* ... */ }
_onDragOver = e => e.preventDefault();
}
// 2) In your original module (tree-view.js):
import { TreeViewDragManager } from './tree-view-drag-manager.js';
export function init(id, options) {
const el = document.getElementById(id);
if (!el) return;
// ... existing keyboard wiring ...
if (options.allowDrag) {
const mgr = new TreeViewDragManager(el, options);
mgr.init();
el._dragManager = mgr;
}
}
export function dispose(id) {
const el = document.getElementById(id);
if (!el) return;
EventHandler.off(el, 'keyup', '.tree-root');
el._dragManager?.dispose();
}
```
Benefits:
- Moves all drag logic (event handlers + helpers) into its own module/class.
- Keeps `init`/`dispose` focused on orchestration.
- Makes drag behavior easier to read, test, and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ArgoZhang
dismissed stale reviews from sourcery-ai[bot] and themself
via
6fe9e0d
ArgoZhang
previously approved these changes
Jul 10, 2025
ArgoZhang
approved these changes
Jul 10, 2025
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.
Issues
close #6216
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Enable configurable drag-and-drop behavior for TreeView items, allowing nodes to be repositioned as children or siblings with preview styles and optional drop cancellation.
New Features:
Enhancements:
Documentation:
Tests:
Summary by Sourcery
Enable configurable drag-and-drop functionality in TreeView, allowing users to reposition nodes as siblings or children with visual previews and custom drop handling.
New Features:
Enhancements:
Documentation:
Tests: