Skip to content

Conversation

@cahrens
Copy link

@cahrens cahrens commented Dec 5, 2013

STUD-879

This also makes dragging up into the beginning of a list easier.

Note that I had to add margins (5 px) to the subsections in the unit test to space things apart a bit. Previously in the test, each unit ended at the same place where the next unit began. This made testing the before/after edge cases impossible. In our application, we have quite a bit of space around subsections and sections, so adding margins to the test cases seemed reasonable to me.

@andy-armstrong Please review the code and check out the branch (git checkout christina/dnd) to manually test it.

Sorry about the whitespace changes in the spec-- IDE reformatting.

I advise adding ?w=1 to the end of the URL to ignore whitespace differences.

@andy-armstrong
Copy link
Contributor

The logic looks reasonable, and my testing all worked. Nice! 👍

P.S. It seems like a lot of logic specifically for the overview component. Have we thought about how to make this into shared draggable logic that can be used by other components. Or is overview.js already used in a shared way?

@cahrens
Copy link
Author

cahrens commented Dec 9, 2013

overview.js is not really shareable at this point (beyond its use on both the overview page and the subsection edit page). The draggabilly library is the reusable part.

No, thought has not been given to how this code code be made more reusable. This was our first foray into dragabilly. The next time we make something draggable, I agree we should look at this code to see what could be pulled out.

@cahrens
Copy link
Author

cahrens commented Dec 9, 2013

Another thing--I am going to let Frances get her PR reviewed and merged first (https://github.com/edx/edx-platform/pull/1896). Then I want to make sure my code still works.

@cahrens
Copy link
Author

cahrens commented Dec 12, 2013

I'm closing this because I now have a branch on top of Frances' changes.

@cahrens cahrens closed this Dec 12, 2013
@benpatterson benpatterson deleted the christina/dnd branch January 21, 2015 13:13
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