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

Pager $watchers will not fire if sum of totalItems and page size stays the same #2096

Closed
wants to merge 1 commit into from

Conversation

@igormalyk
Copy link

igormalyk commented Nov 14, 2014

As mentioned by @c0bra at brianchance@45e1b5c#commitcomment-8565148

if decrement and increment of the totalItems and page size don't change the overall sum within one digest cycle then watchers will not fire. Changing the $watch expression to a function which compares string values fixes this.

@igormalyk igormalyk force-pushed the igormalyk:pager-watchers branch from 79406c5 to 794a85f Nov 14, 2014
…tegers to avoid possible addition mistakes of watching 'a + b' when the sum stays the same.
@igormalyk igormalyk force-pushed the igormalyk:pager-watchers branch from 794a85f to 95aa08e Nov 14, 2014
igormalyk referenced this pull request in brianchance/ng-grid Nov 14, 2014
@brianchance brianchance mentioned this pull request Nov 14, 2014
@igormalyk
Copy link
Author

igormalyk commented Nov 18, 2014

@c0bra, isn't a solution to a problem you mentioned ? Is this commit good to be merged or should it be improved ?

@c0bra
Copy link
Member

c0bra commented Dec 8, 2014

@igormalyk I think the right thing to do would be to have the $watch be on a function that returns an object containing the current page and page-size options, like so:

$scope.$watch(function() {
    return {
        a:  $scope.grid.options.pagingCurrentPage,
        b: $scope.grid.options.pagingPageSize
    };
}, function () { ... })

This jsperf test shows that there's an 80% performance hit by doing explicit string conversions: http://jsperf.com/ui-grid-paging-watch-method

@igormalyk
Copy link
Author

igormalyk commented Dec 8, 2014

Didn't think about performance at that time. You are right, @c0bra solution with objects is much better.

@igormalyk igormalyk closed this Dec 8, 2014
@c0bra
Copy link
Member

c0bra commented Dec 10, 2014

@igormalyk I wasn't intending for you to close this. Switching it over to using an function in the $watch should be pretty straightforward. I still think we need this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.