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

Added a dragging feature #229

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

darkomtc11
Copy link

Plain javascript implementation for dragging by holding the edges of modal or by handle with class ".draggable".

Plain javascript  implementation for dragging by holding the edges of modal or by handle with class ".draggable".
@darkomtc11
Copy link
Author

Known issues:

When multiple modal windows are opened, dragging one moves all of the opened modals.

@maximelafarie
Copy link
Owner

Hi @darkomtc11 and thank you for submitting us this PR. I'll take a look at:

When multiple modal windows are opened, dragging one moves all of the opened modals.

The team and I will check and test it, then we'll come back to you. 😉

Regards 🖖

@maximelafarie maximelafarie added effort2: medium (days) help wanted Testing The current code is being tested or should be tested Work In Progress This is a Work In Progress (WIP) labels Sep 25, 2019
BUG FIX: Multiple opened dragging modals will now move independently when being dragged.
For codeclimate issue fix
@darkomtc11
Copy link
Author

darkomtc11 commented Oct 2, 2019

@maximelafarie Hey,
I have fixed the bug that occurred when dragging modal. It used to drag all of the opened modals, and now it is working properly.

@maximelafarie maximelafarie self-requested a review October 3, 2019 08:00
@maximelafarie
Copy link
Owner

Thank you @darkomtc11! You did a great job! This is a really cool feature and it'll be a pleasure to release it! 🤓

The last thing to do is maybe to add some unit tests about position computing and error handling (if / else statements and callbacks) 🤔 @LouisAugry @marco10024 what do you think about it?

@LouisAugry
Copy link
Collaborator

We attach great importance to unit testing, as you can see on our 100% coverage badge. In order to keep our quality standard, can you implement unit tests please? Otherwise excellent work, and thank you very much for contributing ! 😉✌️

@darkomtc11
Copy link
Author

We attach great importance to unit testing, as you can see on our 100% coverage badge. In order to keep our quality standard, can you implement unit tests please? Otherwise excellent work, and thank you very much for contributing ! 😉✌️

Thanks!
I will try to hack something, since I haven't done unit testing before. :)

@maximelafarie
Copy link
Owner

I will try to hack something, since I haven't done unit testing before. :)

Feel free to inspire yourself from the existing unit tests, especially the tests on the modal "target placement" here 😉

Added 13 unit test methods for next methods:
setPosition,
moveDialog,
startDrag,
elementDrag,
stopDrag
@darkomtc11
Copy link
Author

darkomtc11 commented Oct 3, 2019

I will try to hack something, since I haven't done unit testing before. :)

Feel free to inspire yourself from the existing unit tests, especially the tests on the modal "target placement" here 😉

I have finished unit tests for my functions. All passing, all good, but since I have no experience with this, I advise you to check them in detail.
For example, I wasn't sure about .contains() function in nsmContent. (here)

@darkomtc11
Copy link
Author

darkomtc11 commented Oct 3, 2019

I have fixed all requested changes. I hope it is good now. If there's anything else, I will gladly hop in to change things.

Also, I don't know if it's supposed to work like that, but, after 'npm installing' in inner angular project (ngx-smart-modal), I cannot start outer (full) project. I think you might have some node_modules installed locally and they are maybe missing in package.json.
I always need to delete node_modules from inner project to start outer project.

image

@LouisAugry LouisAugry added this to the 2019 features milestone Oct 7, 2019
@darkomtc11
Copy link
Author

Is there anything else I can do to fasten up the process? I really need this feature. :D
I can create another pull request and make sure I DO NOT alter package-lock.json.

@maximelafarie
Copy link
Owner

There's merge conflicts 😕! Can @LouisAugry and @marco10024 take a look about the conflicts please?

@LouisAugry
Copy link
Collaborator

Conflicts are simple, only need to accept both sides and merge together the diffs

@darkomtc11
Copy link
Author

Conflicts are simple, only need to accept both sides and merge together the diffs

Done.

src/app/demo/main/main.component.scss Outdated Show resolved Hide resolved
src/app/demo/main/main.component.html Outdated Show resolved Hide resolved
@maximelafarie
Copy link
Owner

@darkomtc11 I've let some review comments! We're not far from the release, hang on! 😉
You did an awesome work! 💪When you've fixed the rest, I'll merge your work and release a new version! 👌

@darkomtc11
Copy link
Author

@darkomtc11 I've let some review comments! We're not far from the release, hang on! 😉
You did an awesome work! 💪When you've fixed the rest, I'll merge your work and release a new version! 👌

I have done those, but there are package-lock conflicts. Do you have permission to resolve this? If not, tell me what to do exactly. 7.3.0 version has more packages than 7.2.1. I'm not sure which part of file should I take.

@LouisAugry
Copy link
Collaborator

Just remove the package-lock form your changes

@LouisAugry
Copy link
Collaborator

@darkomtc11 Build failed, can you resolve it please ? 😉After that I think we'll merge it !

Many thanks for your help and your contribution ✌️

@vercel
Copy link

vercel bot commented Feb 3, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/biig/ngx-smart-modal/2fllc1124
✅ Preview: https://ngx-smart-modal-git-fork-darkomtc11-feature-draggable.biig.now.sh

@maximelafarie
Copy link
Owner

@darkomtc11 thank you for rebasing but I already done it locally. 😉 I removed the packages files changes and I'm now ready to merge it! Stay tuned! 🖖

@Dunsteer
Copy link

Any news?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing The current code is being tested or should be tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants