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 ClearOnRemove functionality #232

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

xam8re
Copy link

@xam8re xam8re commented Jun 9, 2017

For customer request, I've implemented the closing of modal when a single item are removed.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.07%) to 73.37% when pulling 170a9ae on xam8re:master into ae10480 on guylabs:master.

@guylabs
Copy link
Owner

guylabs commented Jun 12, 2017

Hi @xam8re

thanks a lot for the contribution but I have a question and some comments before I can do the code review:

  1. The requirement is that when a single item is selected and the user clicks on the remove item it will clear the item and close the modal right?
  2. Can you please use the hideModal() method to close the modal instead of the cancelClick()? As the cancel click should be used then the user presses the cancel button and not when we want to close the modal in another method.
  3. Could you remove the whitespace changes in your commit?
  4. Can you maybe add a simple end to end test (see the tests already implemented).
  5. Please execute the grunt build task and update the pull request with the generated files.

When you did I can check the pull request again and do a review. If you have issues some of these points please don't hesitate to ask me.

Thanks a lot and regards

Guy

@xam8re
Copy link
Author

xam8re commented Jun 14, 2017

Hi @guylabs shure. I've done the edit using modal close. I need implement the test but, under windows it doesn't work.
As soon as I've a it of time I'll test under linux and I commit everithings.

@guylabs
Copy link
Owner

guylabs commented Jun 14, 2017

Ok nice! What does not work in Windows? What is the error? Maybe I can help you fix it.

@RodrigoSFC
Copy link

Good Morning,
When you are using the browser and the input is in the middle of the page, when you click on the input, it will launch the OnMouseDown when the mouse is over an option that can be selected, and the OnMouseDown event ends up selecting the item.

Could they correct?

Thanks for the plugin, it is very good!
Best Regards Rodrigo.

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.

4 participants