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.

Sign up for GitHub

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

Updating select from collection automatically #237

Merged
merged 5 commits into from
Jun 16, 2014
Merged

Updating select from collection automatically #237

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
70d61de
Added handling to update selects automatically after the collection i…
yousefcisco May 16, 2014
5999133
Code style fix
yousefcisco May 16, 2014
8d84286
Changes from comments
yousefcisco May 18, 2014
84a8964
Added tests, updated functionality to remove all event listeners
yousefcisco May 19, 2014
4b596e3
Added missing semicolon
yousefcisco May 19, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions backbone.stickit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,12 +556,12 @@
// Determine if this option is selected.
var isSelected = function() {
if (!isMultiple && optionVal != null && fieldVal != null && optionVal === fieldVal) {
return true
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice. thanks :)

} else if (_.isObject(fieldVal) && _.isEqual(optionVal, fieldVal)) {
return true;
}
return false;
}
};

if (isSelected()) {
option.prop('selected', true);
Expand Down Expand Up @@ -593,7 +593,14 @@
}

// Support Backbone.Collection and deserialize.
if (optList instanceof Backbone.Collection) optList = optList.toJSON();
if (optList instanceof Backbone.Collection) {
// Listen to the collection for all events and trigger an update of the select options.
optList.once('all', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only once? And why all? Anything from even a model event could trigger that....

What about change instead?

Copy link

Choose a reason for hiding this comment

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

Once because the listener is assigned during creation and when it gets triggered it causes it to be recreated I think.

The key events would be add, remove, change and sync i think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once because if we used on then we would get a huge number of listeners set against the collection very quickly since the old listener is not deleted after this function is called again and we don't want to manually manage listeners on the collection since that's up to the dev.
I used all instead of change so we can detect add and remove events without having to set multiple event listener.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I'd rather we do .once('add remove reset') or similar. Triggering on
anything is a little silly IMO.

On Fri, May 16, 2014 at 5:13 PM, Yousef Cisco [email protected]:

In backbone.stickit.js:

@@ -593,7 +593,14 @@
}

   // Support Backbone.Collection and deserialize.

  •  if (optList instanceof Backbone.Collection) optList = optList.toJSON();

  •  if (optList instanceof Backbone.Collection) {

  •    // Listen to the collection for all events and trigger an update of the select options.

  •    optList.once('all', function() {

Once because if we used on then we would get a huge number of listeners
set against the collection very quickly since the old listener is not
deleted after this function is called again and we don't want to manually
manage listeners on the collection since that's up to the dev.
I used all instead of change so we can detect add and remove events
without having to set multiple event listener.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/237/files#r12760037
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I thought it might be overkill but thought it would be worth a refresh on every instance instead of limiting to a few and later on having to expand it to cater to a few edge cases. I can update it tomorrow either way :)

var currentVal = getAttr(model, options.observe, options);
applyViewFn.call(this, options.update, $el, currentVal, model, options);
}, this);
optList = optList.toJSON();
}

if (selectConfig.defaultOption) {
addSelectOptions(["__default__"], $el);
Expand Down