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

Trigger sort on add #103

Merged
merged 2 commits into from
May 30, 2018
Merged

Trigger sort on add #103

merged 2 commits into from
May 30, 2018

Conversation

reubenrybnik
Copy link
Contributor

@reubenrybnik reubenrybnik commented May 27, 2018

This PR addresses #102 by triggering sort when a base collection model is added or updated in a way that causes it to match an associated virtual collection's filter so that "adding" items to the virtual collection mimics the behavior of adding items to a Backbone collection (see backbone.js v1.3.3 lines 910 and 925).

A few additional notes:

  • Sort events from the base collection are not currently re-triggered in the virtual collection unless no comparator is set on the virtual collection (see backbone.virtual-collection.js v0.6.15 line 75), so there should be no circumstances under which this causes duplicate sort events to be fired
  • This change doesn't check option sort for false (see backbone.js v1.3.3 line 852) because the existing sorting code currently doesn't (might be worth updating in some future PR, but I'd rather not handle it in this one)
  • This change doesn't trigger sort on change of sortable attributes because as far as I can tell Backbone does not automatically sort on change under any circumstances
  • This change does trigger sort on a change of filterable attributes that cause the filter conditions to be met when they weren't previously because that "adds" the item to the virtual collection
  • This change doesn't trigger sort on update in the case of merged models with changed comparator properties because the existing sorting code currently doesn't handle sorting on update (Backbone's logic there is a bit weird anyway - it only automatically performs sorting if the comparator is a string property name per code around backbone.js v1.3.3 line 870)
  • As far as I can tell Backbone triggers sort even if all new items end up being sorted to the end of the collection, which is nice for keeping this change simple 😄

@@ -102,6 +102,9 @@ var VirtualCollection = Backbone.VirtualCollection = Backbone.Collection.extend(
this._indexAdd(model);
this.listenTo(model, 'all', this._onAllEvent);
this.trigger('add', model, this, options);
if (this.comparator) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make this an explicit undefined check to conform with other checks on this property in this file and to handle the possibility of a comparator on a property with a name that is an empty string.

Copy link
Owner

@p3drosola p3drosola left a comment

Choose a reason for hiding this comment

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

💯

@p3drosola p3drosola merged commit e89b73a into p3drosola:master May 30, 2018
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.

2 participants