Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Chaining doesn't work #23

Closed
joliss opened this Issue May 13, 2012 · 2 comments

Comments

Projects
None yet
2 participants
Contributor

joliss commented May 13, 2012

Minor issue:

$('.time-field').timepicker().find('...') # this works
$('.time-field').timepicker().timepicker('destroy').find('...') # this doesn't

API methods like 'destroy' seem to return the jQuery object twice, like [$('.time-field'), $('.time-field')].

This is happening on jQuery UI 1.8.20 with jQuery 1.7.2.

I tried digging into the source, but I wasn't able to fix up the return values. I'm not sure why the duplication is happening.

Is this a problem with my app/environment, or can you reproduce it?

Owner

wvega commented May 13, 2012

Hi @joliss,

There seems to be a bug in the way chainable methods are being handled (see L605 - there options would have the name of the API method being called). In this case, the destroy method is not even being considered as a chainable method, but one that should return a a value.

The reason you are seeing a duplicated jQuery object is, I think, that you have two elements with class time-field. The destroy method is called on each of the elements, returns the jQuery object (to preserve the chain), but TimePicker creates an array with the results.

Additionally, the destroy method seems incomplete. I would have to look at it again and define what is the expected result.

On a related, but maybe not that relevant here, I don't know which is better when calling API methods that should return a value over a several TimePicker instances:

  • Return the value of the first element only, or
  • Return an array with the result of calling the method on each one of the elements.

what do you think is the right way? I think jQuery returns the value of the first element, right?

Thank you for reporting this issue. I'll try to address them soon.

Contributor

joliss commented May 13, 2012

Thanks for the reply. Yes, you are right that I have two .time-field elements on the page.

Regarding the best behavior, my intuitive expectation would be to get an array. More importantly though, that's what the datepicker does, so following its example would seem sensible.

I have to admit that looking at the datepicker source leaves me rather confused, so I'm not sure how to implement/fix this.

wvega added a commit that referenced this issue May 27, 2012

Makes next, previous, open, close and destroy methods chainable (Issue
…#23)

The methods now return a reference to the jQuery instance associated with the timepicker.

@wvega wvega closed this May 28, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment