Skip to content

Conversation

@SimoTod
Copy link
Collaborator

@SimoTod SimoTod commented Dec 22, 2024

Closes #4482

Many developer uses x-sort with x-for. In this cases, when x-sort gets processed there's technically no [x-sort:handle] in any of the children even when the x-for template would create element with said attribute.

The MR adds some additional logic (in the real word there will probably just be 1 single tag as a direct children but it takes a more generic approach) to support [x-sort:handle] defined in template tags (likely x-for).

@jan-dh
Copy link

jan-dh commented Apr 18, 2025

Would really be happy to see this get merged

calebporzio and others added 2 commits February 8, 2026 21:50
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extract repeated handle selector string to variable
- Mark new x-for handle test as test.skip to match all other sort tests (CI flakiness)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@calebporzio
Copy link
Collaborator

PR Review: #4483 — Add support for [x-sort:handle] attributes defined in template tags

Type: Bug fix
Verdict: Merge

What's happening (plain English)

  1. When you use x-sort with x-for, the sortable items live inside a <template> tag before Alpine renders them.
  2. At init time, x-sort runs el.querySelector('[x-sort:handle]') on the container (<ul>) to check if handles are in use.
  3. But querySelector on a regular DOM element does NOT search inside <template> content fragments — those children live in a separate DocumentFragment.
  4. So useHandles is always false when handles are defined in x-for templates, and every drag works (not just handle drags).

The fix checks <template> elements' .content as a fallback, which is the correct and minimal approach.

Other approaches considered

  1. Defer the check to drag-start time — by then x-for has rendered the items into the DOM. But SortableJS needs the handle option set at initialization, so this won't work.
  2. Always pass the handle selector and let SortableJS figure it out — but when no handles exist, nothing would be draggable. Not viable.
  3. MutationObserver to re-check after render — over-engineered for what is fundamentally a one-time DOM traversal issue.

The PR's approach (check template content at init) is the laziest correct solution.

Changes Made

  1. Resolved merge conflict — main added wire:sort:handle support since this PR was opened. Merged both changes so template content check also covers wire handles.
  2. Extracted handle selector to variable — the selector string [x-sort\:handle],[wire\:sort\:handle] was repeated. Pulled it into let handleSelector for clarity.
  3. Marked new test as test.skip — all other sort tests are skipped due to CI flakiness with drag simulation. The new test would have the same issue, so it should be skipped too for consistency.

Test Results

  • Regression verified: Reverted the fix, rebuilt, ran test → test fails (dragging <li> body incorrectly sorts items).
  • Fix verified: Restored the fix, rebuilt, ran test → test passes (only handle drags sort items).
  • CI status: Both existing CI checks passed (from original PR submission).
  • Sort spec: 11 tests, 11 pending (all skipped per project convention), 0 failures.

Code Review

  • packages/sort/src/index.js:29-30 — The fix is surgical: 2 lines added to check template content. No unrelated changes.
  • tests/cypress/integration/plugins/sort.spec.js:64-88 — Good test structure: verifies both that non-handle drag does NOT sort and that handle drag DOES sort. Correctly uses x-for with :key binding.
  • The PR also added a non-handle-drag assertion to the existing "can use a custom handle" test (line 52-55), which is reasonable but lives in a skipped test. No harm.

Security

No security concerns identified. This change only affects DOM selector queries at initialization time.

Verdict

Clean, minimal bug fix for a real issue. The root cause is verified, the fix is the simplest correct approach, and the test properly isolates the bug. Community engagement is light (1 supportive comment) but the fix is straightforward enough that it doesn't need extensive discussion.

One note: this PR has been open since Dec 2024 with no maintainer feedback. The contributor's approach was correct from the start — it just needed the merge conflict resolved and minor cleanup.

Recommend merge.


Reviewed by Claude

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants