fix: only initialize useSortable if dragEnabled is true#14261
fix: only initialize useSortable if dragEnabled is true#14261isaacbraun wants to merge 3 commits intodevfrom
useSortable if dragEnabled is true#14261Conversation
| this.mutationObserver?.disconnect(); | ||
| } | ||
|
|
||
| private setUpSorting(): void { |
There was a problem hiding this comment.
This whole function is no longer needed, as the !dragEnabled check is moved to the controller and it would only be calling this.sortable.reset().
There was a problem hiding this comment.
Pull request overview
This PR fixes a drag-and-drop regression in Calcite’s sortable controller by ensuring SortableJS is only initialized for components that actually have dragEnabled set to true, preventing nested Lists/List Items and Blocks from being treated as active drop targets (and avoiding the reported parentNode null crash during drag-over).
Changes:
- Updated
useSortableto always tear down before re-initializing, and to no-op setup whendragEnabledisfalse. - Added a browser-level controller test to verify SortableJS is created/destroyed based on
dragEnabled. - Simplified List/BlockGroup sorting setup to rely on
sortable.reset()now that the controller correctly guards initialization.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/components/src/controllers/useSortable.ts |
Ensures Sortable is torn down before setup and only created when dragEnabled is true. |
packages/components/src/controllers/useSortable.browser.spec.tsx |
Adds coverage verifying SortableJS create/destroy behavior driven by dragEnabled. |
packages/components/src/components/list/list.tsx |
Adjusts sorting setup to always reset (controller now safely handles dragEnabled=false). |
packages/components/src/components/block-group/block-group.tsx |
Removes redundant setUpSorting wrapper and directly resets sortable in relevant lifecycle/update paths. |
driskull
left a comment
There was a problem hiding this comment.
This looks good to me @isaacbraun. Lets let @jcfranco give the thumbs up before merging if possible 👍
| return component.dragEnabled && globalDragState.active; | ||
| } | ||
|
|
||
| async function setUpSortable(component: SortableComponent): Promise<void> { |
There was a problem hiding this comment.
I wonder why this was async before. Hmm 🤔
There was a problem hiding this comment.
I wondered the same 👀
jcfranco
left a comment
There was a problem hiding this comment.
Awesome stuff, @isaacbraun!
Thanks for setting up a spec test for useSortable. Let’s keep beefing it up with more coverage. ✨💪✨
|
|
||
| describe("useSortable", () => { | ||
| class Test extends LitElement { | ||
| dragEnabled = false; |
There was a problem hiding this comment.
Suggestion: You can override this property in mount's afterConnect to avoid the additional DragEnabledTest class.
There was a problem hiding this comment.
Oh nice! I may be missing something, as I had to call reset() after overriding the property in afterConnect. It's probably okay but wanted to call it out in case you knew a better way. See LN43
There was a problem hiding this comment.
Apologies, it looks like my suggestion won't work as I expected due to the connected hook firing prior to the prop override (in retrospect, it makes sense due to the name 😅). An alternative would be to decorate the property (similar to how the controller expects the component to implement) and use a dynamic component + lit HTML templating:
Apologies, it looks like my suggestion won't work as expected since the connected hook fires before the prop override, which in retrospect makes sense given the name. 😅
An alternative would be to decorate the property, similar to how the controller expects the component to implement it, and use mount's dynamicComponents + Lit HTML templating:
import { html } from "lit";
// ...
class Test extends LitElement {
static tagName = "sortable-test";
@property({ type: Boolean }) dragEnabled = false;
// ...
}
// ...
await mount(
html`<sortable-test drag-enabled></sortable-test>`, {
dynamicComponents: [Test]
}
);I’ll defer to you on whether this version or the original fits best.
5758b13 to
6e56e2e
Compare
Related Issue: #14259
Summary
This PR refactors the
useSortablesetUpSortablefunction to always tear down before setting up, and crucially only set it up if the component'sdragEnabledproperty istrue.Without this change, nested List Item instances and Lists inside Blocks are being initialized as Sortable containers regardless of their
dragEnabledvalue, letting Sortable treat them as active drop targets and creating theparentNode nullcrash during drag-over.Additionally, adds a Browser test for the
useSortablecontroller. NOTE: this test is AI generated, I would appreciate a closer review.