Cancel Dragging on ESC key up

[BurakParsAydin]

Thank you for submitting a pull request! Please make sure you have read the contribution guidelines before proceeding.

This PR implements or fixes... (explain your changes)

It implements cancel on ESC key up …

This PR closes the following issues... (if applicable)

Issue-204 …

Does this PR require the Docs to be updated?

I don't think so …

Does this PR require new tests?

…

This branch been tested on... (click all that apply / add new items)

both Mac OS Safari and Linux Chrome. It runs without any error.

Browsers:

• [x] Chrome version
• [ ] Firefox version
• [x] Safari version
• [ ] IE / Edge version
• [ ] iOS Browser version
• [ ] Android Browser version

[bahung1221]

Hi @burakswe ,

Thanks for your solution 🎉 , I just add some minor changes about documentation & test cases.

But I found that the code of cancel method is duplicated with onDragStop method, of course, the method name onDragStop means an event, not a method to call directly but I think it will be better than duplicate and maybe we should create a new method and then call this method in both cancel and onDragStop, maybe dragStop or something similar.

How about this?

  /**
   * Cancel dragging immediately
   */
  cancel() {
      this[dragStop]();
  }

  [onDragStop](event) {
      this[dragStop](event);
  }

  [dragStop](event) {
      // Stop event code here
  }

[BurakParsAydin]

Hi @bahung1221 ,

Thanks for the response. I just reviewed the file changes. I think, I should add tests before every commit and add guidance to the documentation when necessary.

I revised src/Draggable/Draggable.js as you suggested and bind dragStop to constructor. However cancel method throws an error saying "sensorEvent is undefined". I just created a gist file for you to be able to produce the same error. Here is the link: https://gist.github.com/burakswe/abb3a845d4e14cf5a0d33d97992f454e Please replace it with your current file.

Cheers!

[bahung1221]

You're right since the cancel was called manually so we don't have mouse/touch event context here, so just check null for it may enough 🤔

It seems that L572 and L617 are the only two lines that need event data, how about it?

https://gist.github.com/burakswe/abb3a845d4e14cf5a0d33d97992f454e#file-draggable-js-L572

const dragStopEvent = new DragStopEvent({
      source: this.source,
      originalSource: this.originalSource,
      sensorEvent: event ? event.sensorEvent : null,
      sourceContainer: this.sourceContainer,
    });

Cheers 🍻

[BurakParsAydin]

@bahung1221 Thanks for the advice. I just revised the function as you outlined and pushed the commit. Now it works well on my end.

[bahung1221]

Everything seems ok now 🎉

Hi @tsov this PR is ready to merge, please help to merge it when you have free time.

Hello @zjffun , if you have free time, please help to review this PR too, I appreciate any advice from you.

Thanks 🍻