Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "change" from drag-n-drop events #290

Merged

Conversation

michaeljaltamirano
Copy link
Contributor

@michaeljaltamirano michaeljaltamirano commented Apr 7, 2021

Checklist:

  • No linting issues
  • Commits are compliant with commitizen
  • CI tests have passed
  • Documentation updated
    • No updates required, JSDoc comment linking to MDN in source code

Summary of changes

A repository I work on is seeing files uploaded twice as mentioned in #276.

I dug into the local /dist of this library and noticed that removing 'change' from the SUBJECT_TYPE.DRAG_N_DROP values resolved my issue locally. It looks like 'change' is not included in the MDN docs for DragEvent.

It looks like the tests set up in this library are still passing, so I'm curious to see if this fixes people's issues without causing any regressions 🙂

Linked issues

Closes #276

Copy link
Owner

@abramenal abramenal left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I think a small change is needed here, happy to release it as soon as it's ready

src/constants.js Outdated
/**
* @see https://developer.mozilla.org/en-US/docs/Web/API/DragEvent
*/
[SUBJECT_TYPE.DRAG_N_DROP]: [
Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with this change. The only think that IMO important is order – it is weird to have dragend triggered before dragstart

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abramenal Thanks for the quick review! I pushed 8adf3d7 to restore the order in main with dragover added before drop, and dragexit between dragleave and dragend. Happy to make another commit if you have a specific preference of events and order not captured.

Copy link
Owner

@abramenal abramenal Apr 8, 2021

Choose a reason for hiding this comment

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

Cool stuff, now order is good!
I was checking dragover event specs on MDN and have an impression that it is triggered on the element that is dragged into dropzone (or anywhere else), but not the dropzone itself.

https://jsfiddle.net/radonirinamaminiaina/zfnj5rv4/

Added console log there, trying to drop labeled div into any other down below:

Screenshot 2021-04-08 at 6 02 24 PM

Can you please double check that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds (and looks) correct. Should we remove dragexit as well? That was another event I added, and it does not seem strictly applicable to the usage in this PR, so if we're removing dragover I think it makes sense to remove dragexit as well 🙂

EDIT: I've gone ahead and just made the diff removing 'change' in 055e837, I will update the PR description now.

@michaeljaltamirano michaeljaltamirano changed the title Update drag n drop events Remove "change" from drag-n-drop events Apr 8, 2021
@abramenal abramenal merged commit f252664 into abramenal:main Apr 8, 2021
@abramenal
Copy link
Owner

@all-contributors add @michaeljaltamirano for code

@allcontributors
Copy link
Contributor

@abramenal

I've put up a pull request to add @michaeljaltamirano! 🎉

@abramenal
Copy link
Owner

Great job @michaeljaltamirano! Let me release this now

@abramenal
Copy link
Owner

Released in v5.0.5

@8gentile
Copy link

Awesome work gang, thanks!

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.

[Bug] attachFile called once, but posting two files
3 participants