Skip to content

fix(header): allow sorting on mobile devices with reorderable enabled#710

Merged
fh1ch merged 1 commit into
mainfrom
fix/mobile-sorting-reorderable-conflict
Jun 11, 2026
Merged

fix(header): allow sorting on mobile devices with reorderable enabled#710
fh1ch merged 1 commit into
mainfrom
fix/mobile-sorting-reorderable-conflict

Conversation

@spike-rabbit

@spike-rabbit spike-rabbit commented Jun 11, 2026

Copy link
Copy Markdown
Member

Description

On mobile devices, sorting was not working when tapping on a header due to a conflict with the [reorderable] input. The touchstart handler in the draggable directive was calling event.preventDefault() immediately, which prevented the synthetic click event from firing.

Changes

  • Removed preventDefault() from touchstart handler in DatatableDraggableDirective
  • Added setDragging() method to properly manage column.dragging state
  • Added guard if (this.touchId !== undefined) to ensure drag only starts if touch is still active after delay
  • Added test verifying short touch taps don't trigger drag start

How It Works

  • Short tap (before the 500ms drag delay): No drag starts, click event fires normally → sorting works
  • Long press (after the delay): Drag starts, column.dragging is set to true, sort handler skips

Testing

All 191 unit tests pass.

Closes #692

@spike-rabbit spike-rabbit requested a review from a team as a code owner June 11, 2026 07:19
@spike-rabbit spike-rabbit force-pushed the fix/mobile-sorting-reorderable-conflict branch from 13d38a1 to b3c11bd Compare June 11, 2026 07:21

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates the DatatableDraggableDirective to manage dragging state via a new setDragging helper, removes event.preventDefault() on touch start to allow click events to proceed, and adds a corresponding unit test. The review feedback suggests improving the touch-start race condition check by verifying the specific touch identifier instead of just checking if it is defined, and wrapping the unit test's fake timer logic in a try...finally block to prevent test flakiness in case of assertion failures.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread projects/ngx-datatable/src/lib/directives/datatable-draggable.directive.ts Outdated
Comment thread projects/ngx-datatable/src/lib/directives/draggable.directive.spec.ts Outdated
@spike-rabbit spike-rabbit force-pushed the fix/mobile-sorting-reorderable-conflict branch 2 times, most recently from aab2ccc to 0fbf007 Compare June 11, 2026 07:44
@fh1ch fh1ch requested a review from Copilot June 11, 2026 09:06
@fh1ch fh1ch added the bug Something isn't working label Jun 11, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes mobile header sorting when column reordering is enabled by avoiding suppression of the synthetic click generated from touch interactions, while still supporting long-press drag behavior for reordering.

Changes:

  • Removed preventDefault() from touchstart and moved touch cancellation to touchmove once dragging begins.
  • Added setDragging() to manage column.dragging during drag lifecycle so sort can be skipped only when an actual drag starts.
  • Added a unit test for short touch interactions before the drag delay.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
projects/ngx-datatable/src/lib/directives/draggable.directive.spec.ts Adds/adjusts unit tests around delayed touch interactions to ensure short touches don’t start a drag.
projects/ngx-datatable/src/lib/directives/datatable-draggable.directive.ts Updates touch handling to allow click-based sorting on mobile while preserving delayed drag start and drag state.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread projects/ngx-datatable/src/lib/directives/datatable-draggable.directive.ts Outdated
Comment thread projects/ngx-datatable/src/lib/directives/draggable.directive.spec.ts Outdated

@fh1ch fh1ch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

@fh1ch fh1ch force-pushed the fix/mobile-sorting-reorderable-conflict branch from 0fbf007 to 1798a1f Compare June 11, 2026 09:15
The touchstart handler was calling preventDefault() immediately, which
prevented synthetic click events from firing. This blocked sorting when
tapping headers on mobile devices with reorderable enabled.

Removed preventDefault() from touchstart and added proper dragging state
management to the draggable directive.

Closes #692
@spike-rabbit spike-rabbit force-pushed the fix/mobile-sorting-reorderable-conflict branch from 1798a1f to ce774a2 Compare June 11, 2026 09:25
@fh1ch fh1ch added this pull request to the merge queue Jun 11, 2026
Merged via the queue into main with commit c1e1b22 Jun 11, 2026
9 checks passed
@fh1ch fh1ch deleted the fix/mobile-sorting-reorderable-conflict branch June 11, 2026 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Unable to sort columns on mobile devices.

3 participants