rvera / image-picker

Image Picker is a simple jQuery plugin that transforms a select element into a more user friendly graphical interface.
http://rvera.github.com/image-picker
Other
907 stars 216 forks source link

Callbacks have wrong parameters - mismatch with docs #132

Closed mastef closed 5 years ago

mastef commented 5 years ago

The documentation mentions that the callbacks are :

changed: function(select, newValues, oldValues, event){...}
clicked: function(select, picker option, event){...}
selected: function(select, picker option, event){...}

However the current behaviour is actually :

changed: function(newValues, oldValues, event){...}
clicked: function(picker option, event){...}
selected: function(picker option, event){...}

This is because the callbacks are called with .call:

this.opts.changed.call(this.select, old_values, this.selected_values(), original_event);
this.opts.clicked.call(this.picker.select, this, event);
this.opts.changed.call(this.select, old_values, this.selected_values(), original_event);

call() uses the first parameter as the scope. So actually what happens is that the value of select becomes the scope of the function, and not the first parameter.

Fix 1 To fix this ( not sure you'd want to fix this, as it might break existing implementations ), you'd want to change it to :

this.opts.changed.call(this, this.select, old_values, this.selected_values(), original_event);
this.opts.clicked.call(this, this.picker.select, this, event);
this.opts.changed.call(this, this.select, old_values, this.selected_values(), original_event);

Fix 2 It might be easier to fix the documentation instead, and move select to the end of the parameters - this would not break backwards compatibility.

this.opts.changed.call(this.select, old_values, this.selected_values(), original_event, this.select);
this.opts.clicked.call(this.picker.select, this, event, this.picker.select);
this.opts.changed.call(this.select, old_values, this.selected_values(), original_event, this.select);

The docs would have to then be changed to :

changed: function(newValues, oldValues, event, select){...}
clicked: function(picker option, event, select){...}
selected: function(picker option, event, select){...}

Fix 3 Or just change the docs to document the current behaviour, and that the select element is available on this instead of the first parameter.

mastef commented 5 years ago

Additionally the docs are wrong for changed as newValues and oldValues is reversed. function(select, newValues, oldValues, event){...} should be : function(select, oldValues, newValues, event){...}

Humni commented 5 years ago

@mastef Thanks for reporting these issues, I'll get these fixed up before releasing 0.4.0

Humni commented 5 years ago

The docs have been updated to reflect current functionality. d86f9f1 resolved this issue and will be merged into the main docs in the 0.4.0 release.