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

Dragleave #540

Merged
merged 11 commits into from
Aug 15, 2020
Merged

Dragleave #540

merged 11 commits into from
Aug 15, 2020

Conversation

christophe-g
Copy link
Contributor

Added containerClass, a css class added when dragitem hover on a container; fires sortenter when a dragitem enters a container; fires dragleave when a dragitem leaves a container.

makes sure original target is sent with the sortenter and sortstart event;
…ainer; fires sortenter when a dragitem enters a container; fires dragleave when a dragitem leaves a container.
@coveralls
Copy link

coveralls commented Jun 3, 2020

Coverage Status

Coverage decreased (-1.4%) to 76.775% when pulling c307869 on christophe-g:dragleave into c81962e on lukasoppermann:master.

@lukasoppermann lukasoppermann added the feature New feature or enhancement label Jun 5, 2020
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -364,11 +380,33 @@ export default function sortable (sortableElements, options: configuration|objec
originalTarget: target
}
}))

_on(sortableContainer, 'dragleave', e => {
const outTarget = e.releatedTarget || e.fromElement;
Copy link
Owner

Choose a reason for hiding this comment

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

What is outTarget? Can we rename this to make it more clear?

@lukasoppermann
Copy link
Owner

Hey @christophe-g, I love this PR, a really great addition to the package.

I left a couple comments. Additionally could you please add some tests to cover the new "uncovered/untested" lines? https://coveralls.io/builds/31215801/source?filename=src%2Fhtml5sortable.ts#L368

This is really important so that we can make sure future additions will not break this part of the package.

Thank you. 👍

@christophe-g
Copy link
Contributor Author

Thanks a lot for the answer @lukasoppermann

Agreed on the comments - will hopefully have some time next week to add tests (and integrate your remarks).
Cheers,
C.

@lukasoppermann
Copy link
Owner

Great, thank you very much. Looking forward to merge it afterwards. 👍

@lukasoppermann
Copy link
Owner

Hey @christophe-g did you have time to work on this. I would love to merge it once it is updated. 👍

@lukasoppermann
Copy link
Owner

Hey @kaffarell do you think you can get this one patched up so that we can merge it? The way I understand it, it could be a nice addition to the package.

@kaffarell
Copy link
Collaborator

👍 Ok, will try later today...

@kaffarell
Copy link
Collaborator

kaffarell commented Jul 3, 2020

@lukasoppermann I resolved the first two of your comments but I really don't get what @christophe-g meant with outTarget I also couldn't write any tests.
Will try tomorrow another time understanding this

src/html5sortable.ts Show resolved Hide resolved
docs/html5sortable.js Show resolved Hide resolved
src/html5sortable.ts Outdated Show resolved Hide resolved
@lukasoppermann
Copy link
Owner

Hey @kaffarell did you also already write some tests for this new update?
I think if we merge this pr and solve the #548 issue we can create a new release.

Trying to fix package-lock
@lukasoppermann
Copy link
Owner

Hey @kaffarell,
I would love to merge this and release a new version. Did you start on the tests? If so do you have a state of them somewhere?

@kaffarell
Copy link
Collaborator

kaffarell commented Aug 5, 2020

Sorry for the late reply, had a lot to do in the recent weeks. I fixed the typo and added the comments But unfortunately I was not able to write the tests.

@lukasoppermann
Copy link
Owner

Okay, no worries. Did you manually test it though?

@kaffarell
Copy link
Collaborator

Tested it manually now and it is working...

@lukasoppermann lukasoppermann merged commit 7a1975c into lukasoppermann:master Aug 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants