Skip to content

Fixed reordering of collection/composite view bug.#2571

Closed
abzainuddin wants to merge 2 commits intomarionettejs:minorfrom
CodeYellowBV:reindex
Closed

Fixed reordering of collection/composite view bug.#2571
abzainuddin wants to merge 2 commits intomarionettejs:minorfrom
CodeYellowBV:reindex

Conversation

@abzainuddin
Copy link
Copy Markdown

Why
The collection/composite view have the option to reorderOnSort. If this is set to true and the comparator on the collection is changed to sort from asc/desc, the sort on the collection is not correctly rendered in the DOM.

What
The views have a _index attribute which is not updated during the sort if reorderOnSort is set to true. If this is updated after a sort, the sort on the collection is correctly rendered in the DOM.

How
_sortViews() uses the view._index to check if the order has changed. Only if this is true it will trigger resortView(). Depending on reorderOnSort, either reorder() or render() is called. Reorder will append each childViews el in the correct order in the DOM, but not update its _index. On subsequent sorts _sortViews() will use an outdated _index and fail. The expected behavior is described in the test when changing the comparator multiple times. The second added test should reindex the views makes sure the views index is updated.

This only occurs with the option reorderOnSort set to true.
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling c4eb0dd on CodeYellowBV:reindex into f9eece3 on marionettejs:minor.

@ahumphreys87
Copy link
Copy Markdown
Member

thanks for the PR @abzainuddin this has already been fixed here: ahumphreys87@e787839

It is pending release of 2.4.2 which is very close. You can track its progress here: #2532

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.

3 participants