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

Add dropactivate and dropdeactivate events #60

Merged
merged 9 commits into from Aug 7, 2014

Conversation

Projects
None yet
2 participants
@mikekloeden
Contributor

mikekloeden commented Aug 3, 2014

These events will be fired during dragstart/dragend on every dropzone the draggable would be accepted and fixes #54.

I Also added an example page demo/dropzones.html to demonstrate a proper use case.

Mike Klöden
Add dropactivate and dropdeactivate events
These events will be fired during dragstart/dragend on every dropzone the draggable would be accepted.
@taye

This comment has been minimized.

Show comment
Hide comment
@taye

taye Aug 3, 2014

Owner

Wow, thanks for all the work. Unfortunately, I can't accept all of this implementation. I've been working on refactoring and fixing bugs in the drop-related code and a lot of your changes would no longer apply.

I'd happily accept ./demo/dropzones.html if it were in a separate commit and the indentation were 4 spaces (splitting it into 3 files - html, css and js - would also be nice but one file is ok). The only changes in ./interact.js that wouldn't cause conflicts are the changes to event binding at lines 360, 3036 and 3042. I can't take the other changes.

If you'd still like to help, I'll let you know when I've finished the refactor (should be very soon) and outline the points of improvement so that you can implement the new events with those.

Owner

taye commented Aug 3, 2014

Wow, thanks for all the work. Unfortunately, I can't accept all of this implementation. I've been working on refactoring and fixing bugs in the drop-related code and a lot of your changes would no longer apply.

I'd happily accept ./demo/dropzones.html if it were in a separate commit and the indentation were 4 spaces (splitting it into 3 files - html, css and js - would also be nice but one file is ok). The only changes in ./interact.js that wouldn't cause conflicts are the changes to event binding at lines 360, 3036 and 3042. I can't take the other changes.

If you'd still like to help, I'll let you know when I've finished the refactor (should be very soon) and outline the points of improvement so that you can implement the new events with those.

@taye

This comment has been minimized.

Show comment
Hide comment
@taye

taye Aug 3, 2014

Owner

I'd happily accept ./demo/dropzones.html if it were in a separate commit and the indentation were 4 spaces...

When I saw lines 68 onwards of the file, I thought that you had been using 8 spaes for indentation throughout the file. I see now that you just indented twice there. Reducing that by one level would be more consistent with how I've done it in other places but I don't mind leaving the indentation as it is.

Owner

taye commented Aug 3, 2014

I'd happily accept ./demo/dropzones.html if it were in a separate commit and the indentation were 4 spaces...

When I saw lines 68 onwards of the file, I thought that you had been using 8 spaes for indentation throughout the file. I see now that you just indented twice there. Reducing that by one level would be more consistent with how I've done it in other places but I don't mind leaving the indentation as it is.

Mike Klöden added some commits Aug 3, 2014

Mike Klöden
Fix dropzone demo
Tidy up HTML by moving CSS and JS to separate files and fix indentation levels.
Mike Klöden
Keep non conflict changes
Remove dropactivate/dropdeactivate logic but keep event bindings for the new events.
@mikekloeden

This comment has been minimized.

Show comment
Hide comment
@mikekloeden

mikekloeden Aug 3, 2014

Contributor

Thanks for your feedback! No problem, I tidied up the demo page. Furthermore I removed everything despite the event bindings for the new events (as you said they won't cause any problems).

I kept my changes regarding the new events in a separate branch now.
So I hope my current master branch looks good now?

Sure, I would like to help if you finished your drop refactoring.
Just give me a hint when you are ready and a short note where I have to look, so I can implement the new events.

Contributor

mikekloeden commented Aug 3, 2014

Thanks for your feedback! No problem, I tidied up the demo page. Furthermore I removed everything despite the event bindings for the new events (as you said they won't cause any problems).

I kept my changes regarding the new events in a separate branch now.
So I hope my current master branch looks good now?

Sure, I would like to help if you finished your drop refactoring.
Just give me a hint when you are ready and a short note where I have to look, so I can implement the new events.

Mike Klöden
Merge remote-tracking branch 'upstream/master'
* upstream/master:
  Fix the options test
  Update tests for gesture events to be less broken
  Remove trailing whitespace
  Make a small change to avoid a jshint warning
  Fix interact.isSet
  Update 'pointerWasMoved' even with no 'prepared'
  Fix Interactable#preventDefault
  Record Touchevents in addition to PointerEvents
@taye

This comment has been minimized.

Show comment
Hide comment
@taye

taye Aug 4, 2014

Owner

I've merged #61 so if you merge my master branch into this, you can work on the (de)activate events with the cleaner code.

Line 2376: if you want to create the events in getDropEvents, then dragMove would probably need to pass starting as the second parameter.

activeDrops contains all the possible dropzones and the elements.

activeDrops     = {
    dropzones: [],      // the dropzones that are mentioned below
    elements : [],      // elements of dropzones that accept the target draggable
    rects    : []       // the rects of the elements mentioned above
},

activeDrops.dropzones repeats dropzones if they have multiple elements. activeDrops.elements repeats elements if they match selectors of multiple dropzones.

Identical dropzones are all adjacent in the dropzones array. When iterating through and firing events for each dropzone, you could compare the current dropzone interactable to the previous one and create a new event if they are different.

Those are my tips. Feel free to do it however you think is best :).

Owner

taye commented Aug 4, 2014

I've merged #61 so if you merge my master branch into this, you can work on the (de)activate events with the cleaner code.

Line 2376: if you want to create the events in getDropEvents, then dragMove would probably need to pass starting as the second parameter.

activeDrops contains all the possible dropzones and the elements.

activeDrops     = {
    dropzones: [],      // the dropzones that are mentioned below
    elements : [],      // elements of dropzones that accept the target draggable
    rects    : []       // the rects of the elements mentioned above
},

activeDrops.dropzones repeats dropzones if they have multiple elements. activeDrops.elements repeats elements if they match selectors of multiple dropzones.

Identical dropzones are all adjacent in the dropzones array. When iterating through and firing events for each dropzone, you could compare the current dropzone interactable to the previous one and create a new event if they are different.

Those are my tips. Feel free to do it however you think is best :).

Mike Klöden added some commits Aug 4, 2014

Mike Klöden
Merge remote-tracking branch 'remotes/upstream/master'
* remotes/upstream/master:
  Tidy and compact getDrop
  Fix a bug in the default Interactable#getRect
  Add and use the 'getDropEvents' function
  Try again to fix dynamicDrop
  Collect possible and actual drops separately
  Fix the 'dynamicDrop' setting
  Combine checks for selector and element dropzones
  Add element argument to Interactable#getRect
  Reorganise the process for getting drops
  Fix enter/leave with same draggable and dropzone

Conflicts:
	interact.js
Mike Klöden
Merge remote-tracking branch 'upstream/master'
* upstream/master:
  Add snap actions array to docs
  Add 'interact.js' and 'drag and drop' keywords
Mike Klöden
Improve dropzone demo to show more scenarios
Setup dropzones with different accept options.
Highlight draggable if it can be dropped.
Add a draggable that can't be dropped.
@mikekloeden

This comment has been minimized.

Show comment
Hide comment
@mikekloeden

mikekloeden Aug 7, 2014

Contributor

I extended the getDropEvents method to return an activate and deactivate event if appropriate. Also a new parameter starting was added (as you already mentioned) to check if the drag just started.

To fire the new events on all active dropzones I created a new fireActiveDrops method which gets an InteractEvent as a parameter. It loops through all active dropzones and checks if the current dropzone element wasn't already processed.
Then the event.target is set to the current dropzone element and the event is fired on the dropzone interactable.

I hope it was fine to introduce a new private method, because the logic for both events is exactly the same besides the event to be fired.
If the name doesn't fit into your naming convention feel free to change it :)

Last but not least I updated the dropzones demo.

Contributor

mikekloeden commented Aug 7, 2014

I extended the getDropEvents method to return an activate and deactivate event if appropriate. Also a new parameter starting was added (as you already mentioned) to check if the drag just started.

To fire the new events on all active dropzones I created a new fireActiveDrops method which gets an InteractEvent as a parameter. It loops through all active dropzones and checks if the current dropzone element wasn't already processed.
Then the event.target is set to the current dropzone element and the event is fired on the dropzone interactable.

I hope it was fine to introduce a new private method, because the logic for both events is exactly the same besides the event to be fired.
If the name doesn't fit into your naming convention feel free to change it :)

Last but not least I updated the dropzones demo.

Mike Klöden
Merge remote-tracking branch 'upstream/master'
* upstream/master:
  Pass pointer event to getDropEvents
  dropCheck argument

Conflicts:
	interact.js
@taye

This comment has been minimized.

Show comment
Hide comment
@taye

taye Aug 7, 2014

Owner

Beautiful! fireActiveDrops is great and the updated demo is very nice.

Owner

taye commented Aug 7, 2014

Beautiful! fireActiveDrops is great and the updated demo is very nice.

taye added a commit that referenced this pull request Aug 7, 2014

Merge pull request #60 from mikekloeden/master
Add dropactivate and dropdeactivate events

@taye taye merged commit 1bc60d2 into taye:master Aug 7, 2014

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