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

Fix: set value to null when clearing from a single select select2 and null check in ngOnDestroy #74

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

Conversation

tjwoods58
Copy link

@tjwoods58 tjwoods58 commented Mar 29, 2017

Closes #61

@NejcZdovc NejcZdovc self-requested a review March 29, 2017 23:12
@piernik
Copy link

piernik commented Aug 22, 2017

Is this fix gonna be published?

@ricavir11
Copy link

Hi, I'm also wondering when this fix will be published... It would be great !

@MarkHerhold
Copy link

@NejcZdovc Do you have any plans to merge this?

@NejcZdovc
Copy link
Owner

@MarkHerhold did you try out hits patch? Is it working for you with this fix?

@MarkHerhold
Copy link

@NejcZdovc Partially. I can't directly reproduce the issue but I know that the if check will only prevent potential errors. I would actually go further than this PR does and check that off is a function and not just that it exists.

I can't comment on the other portion of this PR as we don't have issues with the select/unselect events.

I will submit a PR that only adds the if check as that's what I can validate and I know it's a safe change. Are you OK with this @NejcZdovc ?

@NejcZdovc
Copy link
Owner

@MarkHerhold sounds good

@sbanerjee302
Copy link
Contributor

@NejcZdovc can you please merge this fix also ? facing a similar issue.

@NejcZdovc
Copy link
Owner

@sbanerjee302 can you please test this PR out and confirm that it's working correctly

@NejcZdovc NejcZdovc closed this Jun 22, 2018
@NejcZdovc NejcZdovc reopened this Jun 22, 2018
@sbanerjee302
Copy link
Contributor

@NejcZdovc The null check in ngOnDestroy is definitely working which was the issue I was facing.

@NejcZdovc
Copy link
Owner

@MarkHerhold said that he will do a pr just for this null check, so we can merge that one

@sbanerjee302
Copy link
Contributor

@NejcZdovc I have created a PR for the null check and also added a check if the off function exists. Can you please review.

@NejcZdovc
Copy link
Owner

NejcZdovc commented Jun 22, 2018

@sbanerjee302 done, merged and new version published [email protected]

@NejcZdovc NejcZdovc removed their request for review June 18, 2020 07:12
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.

6 participants