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

Remove listen changeDate after destroy DateRangePicker #1968

Merged
merged 1 commit into from
Aug 28, 2016
Merged

Remove listen changeDate after destroy DateRangePicker #1968

merged 1 commit into from
Aug 28, 2016

Conversation

lusever
Copy link
Contributor

@lusever lusever commented Aug 4, 2016

Need for correct recreate DateRangePicker on same inputs.

@@ -1567,6 +1567,7 @@
},
destroy: function(){
$.map(this.pickers, function(p){ p.destroy(); });
$(this.inputs).off('changeDate', this.dateUpdated);
Copy link
Member

Choose a reason for hiding this comment

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

this.dateUpdated !== $.proxy(this.dateUpdated, this);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But it is work for jQuery. From docs:

When jQuery attaches an event handler, it assigns a unique id to the handler function. Handlers proxied by jQuery.proxy() or a similar mechanism will all have the same unique id (the proxy function), so passing proxied handlers to .off may remove more handlers than intended. In those situations it is better to attach and remove event handlers using namespaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

И чего?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/eternicode/bootstrap-datepicker/blob/master/js/bootstrap-datepicker.js#L2001 используется нэймспейс. Я считаю что лучше использовать нэймспейс, даже jQuery советует.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the way of removing events to namespace.

@vsn4ik
Copy link
Member

vsn4ik commented Aug 22, 2016

@lusever
Copy link
Contributor Author

lusever commented Aug 24, 2016

I added namespace.
@vsn4ik prompt me please place where to create the testcase correctly.

@acrobat
Copy link
Member

acrobat commented Aug 24, 2016

@vsn4ik I think we can't add a namespace now, this would mean a BC break. We can only change the namespace in 2.0 (We can also fix issue like this only when we develop 2.0 #337). Why is the namespace necessary?

@vsn4ik
Copy link
Member

vsn4ik commented Aug 25, 2016

@acrobat Let's roll out without tests. Namespace is not necessary.

@acrobat
Copy link
Member

acrobat commented Aug 25, 2016

@lusever Can you remove the .datepicker namespace on both events? If that still fixes your issue the pr is good to go!

@lusever
Copy link
Contributor Author

lusever commented Aug 25, 2016

@acrobat I removed namespaces.

@lusever
Copy link
Contributor Author

lusever commented Aug 25, 2016

If I use the code which is currently:

$('.inputs input').on('changeDate', myFunc)
$('.inputs').datepicker({inputs: $('input')})
$('.inputs').data('datepicker').remove(); // removed myFunc too

I think the original version is more correct. It is like in _unapplyEvents.
I think it is necessary to restore the original version. Then myFunc will not be removed.

@acrobat
Copy link
Member

acrobat commented Aug 26, 2016

So the changes in this PR are not correct @lusever?

@lusever
Copy link
Contributor Author

lusever commented Aug 26, 2016

@acrobat now is correct.

@acrobat
Copy link
Member

acrobat commented Aug 28, 2016

Thanks @lusever!

@acrobat acrobat merged commit fa0a0ec into uxsolutions:master Aug 28, 2016
@acrobat acrobat added this to the 1.7.0 milestone Aug 28, 2016
@acrobat acrobat mentioned this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants