adopted-ember-addons / ember-sortable

Sortable UI primitives for Ember.js
https://ember-sortable.netlify.app/
MIT License
298 stars 149 forks source link

Prevent click event on drop #405

Open schorke opened 3 years ago

schorke commented 3 years ago

Describe the bug Click event inside a sortable-item is triggered on drop.

To Reproduce

import Component from '@glimmer/component';
import { action } from '@ember/object';
import { tracked } from '@glimmer/tracking';
import { A } from '@ember/array';

export default class DragList extends Component {
  @tracked
  _items = [ 'Uno', 'Dos', 'Tres', 'Cuatro', 'Cinco' ];

  get items() {
    return this._items;
  }

  @action
  update(newOrder) {
    this._items = A(newOrder);
  }

  @action
  rowClicked() {
    console.log('clicked');
  }
}
<ol {{sortable-group groupName="group1" onChange=this.update}}>
  {{#each this.items as |item|}}
    <div {{on "click" this.rowClicked}}>
      <li {{sortable-item groupName="group1" model=item}} >
        {{item}}
      </li>
    </div>
  {{/each}}
</ol>

=> This will not trigger the rowClicked action, which is the expected behaviour here since there is this code which will not propagate / bubble up the event.

<ol {{sortable-group groupName="group2" onChange=this.update}}>
  {{#each this.items as |item|}}
    <li {{sortable-item groupName="group2" model=item}} >
      <div {{on "click" this.rowClicked}}>
        {{item}}
      </div>
    </li>
  {{/each}}
</ol>

=> This will trigger the rowClicked action, which I don't want to happen. Is this a known / expected behaviour ? Do you have any fix / workaround ?

My workaround is to assert parent's class is-dropping in the click event like this:

@action
rowClicked(e) {
  if (!e.target.parentElement.classList.contains('is-dropping')) {
    console.log('clicked');
  }
}

Expected behavior Click event inside sortable-item should not be triggered when dropping it. We previously used ember-drag-drop which had this behaviour built in.

Other info

ygongdev commented 3 years ago

I can take a look, but just 2 things from my side:

  1. From a purely performance standpoint, it seems like using event delegation here would be more ideal, e.g attach click listener on the ol and attach an identifier (e.g data-*) on each row.

  2. if you were to use a handle, it should solve this problem and perhaps provide a better a11y experience as well.

MelSumner commented 2 years ago

@ygongdev were you able to look at this issue?

Additionally, I wonder if we should implement improved documentation and guidance? It seems like OP should refactor a bit instead of adjusting the addon behavior. WDYT?