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

fix: events can not be dragged back to their original location #847

Merged
merged 3 commits into from
Feb 3, 2019
Merged

fix: events can not be dragged back to their original location #847

merged 3 commits into from
Feb 3, 2019

Conversation

Hapcy
Copy link
Contributor

@Hapcy Hapcy commented Jan 10, 2019

When you start dragging an event on any views it works fine, but then when you try to move it back to the original position it skips over it (you can try this in the kitchen sink).

The problem appeared when the threshold for differentiating between a drag and a click (with clumsy minor drag) has been introduced. Returning the event to the original position causes the validateDrag function to return false. To solve this I introduced a dragAlreadyMoved field which turns to true when a single dragging event has happened (meaning that it has been over the threshold at least once). When the dragAlreadyMoved flag is true we no longer care about the threshold. This had to be implemented for week view normal events and all day events and for day view events. I fixed the daily scheduler demo aswell.

I tried a few ways of solving the problem.
By saving the previous drag position and comparing to that but it required way more work in the components.
I tried using local variables (in closure scope) instead of fields for dragAlreadyMoved but it was less clear.
An alternative solution could be modifying the draggable library to call the validateDrag function with the x-y coordinates which would be used in the not snapping mode instead of the snap coordinates, but that could probably break many things because it seems really fundamental to how the draggable library works.

@codecov-io
Copy link

codecov-io commented Jan 10, 2019

Codecov Report

Merging #847 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #847      +/-   ##
==========================================
+ Coverage   96.99%   97.02%   +0.03%     
==========================================
  Files          38       38              
  Lines         864      874      +10     
  Branches      118      119       +1     
==========================================
+ Hits          838      848      +10     
  Partials       26       26
Impacted Files Coverage Δ
...r/src/modules/week/calendar-week-view.component.ts 96.1% <100%> (+0.08%) ⬆️
...rc/modules/common/calendar-drag-helper.provider.ts 100% <100%> (ø) ⬆️
...dar/src/modules/day/calendar-day-view.component.ts 95.93% <100%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65d354b...262696d. Read the comment docs.

Copy link
Owner

@mattlewis92 mattlewis92 left a comment

Choose a reason for hiding this comment

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

Apologies for the extremely late reply on this. I just had a look over this PR, and it looks absolutely perfect to me, nice job! ❤️

@mattlewis92 mattlewis92 changed the base branch from master to 0.27 February 3, 2019 07:48
@mattlewis92 mattlewis92 merged commit 0f69010 into mattlewis92:0.27 Feb 3, 2019
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.

3 participants