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

Fixed Copy:true elements are not sortable after drop #337

Merged
merged 1 commit into from
Mar 6, 2018
Merged

Fixed Copy:true elements are not sortable after drop #337

merged 1 commit into from
Mar 6, 2018

Conversation

mfriesen
Copy link
Contributor

@mfriesen mfriesen commented Mar 2, 2018

This Pull Request fixes the #311 issue.

I changed the drag/drop events to work at the SortableElement level, so when elements are added to a SortableElement, the automatically inherit all events.

@coveralls
Copy link

coveralls commented Mar 2, 2018

Coverage Status

Coverage increased (+5.2%) to 84.11% when pulling 39f56b9 on mfriesen:copyfix into bbaaab8 on lukasoppermann:master.

@lukasoppermann
Copy link
Owner

lukasoppermann commented Mar 2, 2018

Hey @mfriesen,

thank you very much. It is a nice idea!
Sadly however it currently breaks the lists with handles (the blue white orange one) as well as the sortable table.

It might be due to using e.target, this can be an issue when the target is the td or the handle instead of the whole list element.

One way of solving this could be similar to #338 and adding a is-sortable-item attribute to every list item.

@mfriesen
Copy link
Contributor Author

mfriesen commented Mar 3, 2018

@lukasoppermann Have a look now. I think I got everything working. I added logic to find the correct sortable from the e.target

@lukasoppermann
Copy link
Owner

Nice, will have a look as soon as possible, thank you.

@@ -560,6 +595,8 @@ export default function sortable (sortableElements, options) {
}, options.debounce)
// Handle dragover and dragenter events on draggable items
var onDragOverEnter = function (e) {
var element = e.target
Copy link
Owner

@lukasoppermann lukasoppermann Mar 4, 2018

Choose a reason for hiding this comment

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

This element is a <td> in tables, which is why it does not work for tables. We need to this because it means tables currently don't work.

Maybe you can just use .closest(options.item) because closest should return the item, if it matches the selector and go up the dom tree if it does not.

Copy link
Owner

Choose a reason for hiding this comment

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

This is still a problem, as it still returns <td>

@lukasoppermann
Copy link
Owner

lukasoppermann commented Mar 4, 2018

Hey, so I merged #343 which sadly makes this PR "break".

However the adjustments are minor:

  1. _getChildren was removed, use .children instead, e.g. sortableElement.children
  2. testing framework has been changed to Jest. All tests in the test folder have been moved to __tests__ and "updated" to work with jest. Those are minor updates, so you would just have to add your changes to test/events.js to the __tests__/events.test.ts file.

Sorry for the inconvenience, I just needed to merge this so #259 can be started. Looking forward to your update.

Maybe you can just use .closest(options.item) or something.

@mfriesen
Copy link
Contributor Author

mfriesen commented Mar 4, 2018

@lukasoppermann I've fixed the branch

@lukasoppermann
Copy link
Owner

Hey, sorry, but I think it is not fixed yet. The table still does not work, as e.target is a <td> but needs to be a <tr>.

@mfriesen
Copy link
Contributor Author

mfriesen commented Mar 5, 2018

@lukasoppermann I think it's all fixed now.

@lukasoppermann lukasoppermann merged commit 8922e30 into lukasoppermann:master Mar 6, 2018
@lukasoppermann
Copy link
Owner

Neat, thank you. I will give the master branch a bit more manual testing and than we can build another release with the copy feature included. 👍

@mfriesen mfriesen deleted the copyfix branch March 6, 2018 17:09
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