Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix logic in Paginator hasNextPage() method. #1467

Open
wants to merge 1 commit into from

3 participants

@onlywei

Previously was not working when totalItems is 0.

@lsmith lsmith commented on the diff
src/paginator/js/paginator-core.js
@@ -117,7 +117,11 @@ Y.mix(PaginatorCore.prototype, {
@return {Boolean} `true` if there is a next page, `false` otherwise.
*/
hasNextPage: function () {
- return (!this.get('totalItems') || this.get('page') < this.get('totalPages'));
@lsmith
lsmith added a note

Or return this.get('totalItems') && (this.get('page') < this.get('totalPages'));

@lsmith
lsmith added a note

Uh oh. I misunderstood the intent of the code.

You can see in the API docs for the method that if 'totalItems' isn't set, Paginator assumes there is always a next page. I guess this is to simulate infinite result sets.

@apipkin
apipkin added a note

@lsmith That's correct. In the event that it is bound to a datasource with an unknown upper limit (ie a search page, or email inbox)

@onlywei
onlywei added a note

It doesn't look like I can check if this.get('totalItems') === 0 either because 0 is "unset" for that attribute, right?

Searches returning 0 results is a very real case, though. Need a way to deal with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/paginator/HISTORY.md
@@ -4,7 +4,7 @@ Paginator Change History
@VERSION@
------
-* No changes.
+* Fix the logic in the hasNextPage() method.
@lsmith
lsmith added a note

Please be specific about what was fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@onlywei

I'm having trouble running yogi test for this module, how about you?

Update: works now after running npm install in the root directory.

src/paginator/HISTORY.md
@@ -4,7 +4,7 @@ Paginator Change History
@VERSION@
------
-* No changes.
+* Fix hasNextPage() method. Previously failing when totalItems = 0.
@lsmith
lsmith added a note

"* hasNextPage() now correctly reports false when there are no items to be paged over."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@apipkin apipkin was assigned
@onlywei onlywei Paginator hasNextPage() now correctly reports false when there are no
items to be paged over.

Previously was not working when totalItems is 0.
9d341a8
@lsmith

@onlywei I would suggest opening an issue for this and discuss with @apipkin. It looks like the code is working as designed, but does not distinguish the "no items" case from the "unbounded items" case.

@apipkin apipkin removed their assignment
@apipkin apipkin was assigned by okuryu
@apipkin apipkin was unassigned by okuryu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 6, 2013
  1. @onlywei

    Paginator hasNextPage() now correctly reports false when there are no

    onlywei authored Wei Wang committed
    items to be paged over.
    
    Previously was not working when totalItems is 0.
This page is out of date. Refresh to see the latest.
View
2  src/paginator/HISTORY.md
@@ -4,7 +4,7 @@ Paginator Change History
@VERSION@
------
-* No changes.
+* hasNextPage() now correctly reports false when there are no items to be paged over.
3.14.0
------
View
2  src/paginator/js/paginator-core.js
@@ -117,7 +117,7 @@ Y.mix(PaginatorCore.prototype, {
@return {Boolean} `true` if there is a next page, `false` otherwise.
*/
hasNextPage: function () {
- return (!this.get('totalItems') || this.get('page') < this.get('totalPages'));
@lsmith
lsmith added a note

Or return this.get('totalItems') && (this.get('page') < this.get('totalPages'));

@lsmith
lsmith added a note

Uh oh. I misunderstood the intent of the code.

You can see in the API docs for the method that if 'totalItems' isn't set, Paginator assumes there is always a next page. I guess this is to simulate infinite result sets.

@apipkin
apipkin added a note

@lsmith That's correct. In the event that it is bound to a datasource with an unknown upper limit (ie a search page, or email inbox)

@onlywei
onlywei added a note

It doesn't look like I can check if this.get('totalItems') === 0 either because 0 is "unset" for that attribute, right?

Searches returning 0 results is a very real case, though. Need a way to deal with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ return !!this.get('totalItems') && this.get('page') < this.get('totalPages');
},
View
15 src/paginator/tests/unit/assets/paginator-tests.js
@@ -25,6 +25,21 @@ suite.add(new Y.Test.Case({
},
+ 'test hasNextPage': function () {
+ var pg = new Y.Paginator({itemsPerPage: 10, totalItems: 100});
+
+ Y.Assert.isTrue(pg.hasNextPage(), 'hasNextPage() fail on page 1 of 10.');
+
+ pg.set('page', 9);
+ Y.Assert.isTrue(pg.hasNextPage(), 'hasNextPage() fail on page 9 of 10.');
+
+ pg.set('page', 10);
+ Y.Assert.isFalse(pg.hasNextPage(), 'hasNextPage() fail on page 10 of 10.');
+
+ pg.set('totalItems', 0);
+ Y.Assert.isFalse(pg.hasNextPage(), 'hasNextPage() fail when totalItems is 0.');
+ },
+
'test event name': function () {
var test = this,
pg = new Y.Paginator({
Something went wrong with that request. Please try again.