Skip to content

Draggable: modified snapping algorithm to use edges and corners. Fixed #8165 - Draggable: Snapping doesn't take top/left into account properly #796

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

Closed
wants to merge 346 commits into from

Conversation

zbigniewmotyka
Copy link

Fixed #8165 - Draggable: Snapping doesn't take top/left into account properly

@scottgonzalez
Copy link
Member

Is this a duplicate of #780?

@scottgonzalez
Copy link
Member

The code here is certainly a lot smaller. What's the technical difference?

@zbigniewmotyka
Copy link
Author

In #780 condition checks if at least one of corners or edges is in area which should cause snapping.
In my commit condition checks if there is at least one condition which shouldn't cause snapping (top edge of rectangle 1 is too far from bottom edge of rectangle 2, or bottom edge of rectangle 1 is too far from top edge of rectangle 2, or left edge of rectangle 1 is too far from right edge of rectangle 2, or right edge of rectangle 1 is too far from left edge of rectangle 2).

@mikesherov
Copy link
Member

Hi @zmszaman, thanks again for contributing this patch. We recently re-enabled the test suite for draggable. In order for us to land this patch, we'd need a few tests added to the test suite proving this doesn't break existing functionality and also that it fixes the bug as described. Can you add some tests here please?

@zbigniewmotyka
Copy link
Author

I ran tests and it doesn't break existing functionality. Every tests passes.
I will write test for this bug as soon as I find some free time.

@mikesherov
Copy link
Member

@zmszaman, thanks again for contributing the patch! I'm eager to land a fix. Just to recap what we need to land this pull request:

  1. unit tests showing this fixing the bug.
  2. remove the "yes, I know this is insane ;)" comment and add a better one explaining what you explained to @scottgonzalez here: Draggable: modified snapping algorithm to use edges and corners. Fixed #8165 - Draggable: Snapping doesn't take top/left into account properly #796 (comment)
  3. sign the CLA: http://jquery.github.com/cla.html

Please let me know when you've completed that! Thanks again.

@mikesherov
Copy link
Member

@zmszaman, I can write the tests if you can't find the time, but please sign http://jquery.github.com/cla.html so I can land this patch.

@zbigniewmotyka
Copy link
Author

I just sign CLA. I will be grateful if you write the test.

@mikesherov
Copy link
Member

@zmszaman, can you rebase this code, it's not going to merge cleanly at this point.

jzaefferer and others added 20 commits March 13, 2013 09:50
…. Fixes #6830 - Allow Icons to be specified for Dialog buttons.
…ontent, then buttonpane, then close button, then dialog. Fixes #4731 - Dialog: Add option to set which element gains focus on open
…able methods. Disabling dialogs is not supported.
… check. Fixes #8824 - Deprecate array notation for position option.
…8838 - Dialog: Close icon does not work in dialog larger than the window in IE.
…o prevent XSS. Fixes #6016 - Dialog: Title XSS Vulnerability.
…ixed #7960 - Dialog: Modal dialogs do not disable resizables on the page.
tjvantoll and others added 25 commits March 13, 2013 09:51
…er from an image. Fixes #5129 - Sortable: Unable to use an image as a placeholder in Firefox.
…nal functions when proxying, don't rename originals.
… floating. Fixes #8792: Issue with floated items in connected lists.
…. Fixes #9041: the over event fires on every pixel movement
…es #9150 - CSS Framework: Add ui-icon-blank.

This essentially reverts 1fe06f0 and the fix for #5659.
… disabled option.

Fixes #5973 - Resizable: disabled should not have the ui-state-disabled class or aria attribute aria-disabled
Fixes #5974 - Draggable: disabled should not have the ui-state-disabled class or aria attribute aria-disabled
Fixes #6039 - Droppable : disabled should not have ui-state-disabled
…unt-css and grunt-html, update custom tasks. Drop qunit-junit plugin, not worth the trouble. Update release script to run grunt-prepare after npm-install.
…ds a new version of grunt-compare-size to actuall work
@zbigniewmotyka
Copy link
Author

I did rebase on the code

@mikesherov
Copy link
Member

@zmszaman, please try again. First pull latest upstream master, then rebase your branch against latest master. Looks like you rebased against an old master.

@mikesherov
Copy link
Member

OK, I'll go ahead and rebase this for you when I land it.

@mikesherov
Copy link
Member

Thanks for contributing! Patch landed in bd126a9

For future reference, if you'd like to contribute more patches:

  1. Make sure your git name and email match the records you entered into our CLA.
  2. Make sure you submit a unit test supporting your change.
  3. Make sure you don't develop in the master branch.... as you can see, it's hard to rebase from master.

Thanks again!

@mikesherov mikesherov closed this Mar 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.