pfPagination: pagination controls #514
Conversation
dlabrecq
left a comment
dlabrecq
left a comment
There was a problem hiding this comment.
Other than my pagination service question, everything else looks good to me.
src/pagination/pagination.service.js
Outdated
| @@ -0,0 +1,24 @@ | |||
| (function () { | |||
There was a problem hiding this comment.
It appears that the pagination service isn't currently used by the pagination component? (And the setPages function isn't implemented in the service itself.) Perhaps we should omit this until we can show an example that works with table and list view, for example?
There was a problem hiding this comment.
Hmm..yeah, as the comment in there says it's a placeholder because we may need a service to hold variables to hold the current pagination configs (pageSize, pageNum, etc..) when switching between List, Card, and Table views, but I won't know until I integrate them.
I'll remove the service for now and add it later if needed -thanks
cdcabrera
left a comment
cdcabrera
left a comment
There was a problem hiding this comment.
2 questions and a slight behavior modification on the input field
| * @param {number} pageNumber The current page number | ||
| * @param {number} numTotalItems The total number of items in the data set. When a filter is applied, update the <code>numTotalItems</code> | ||
| * accordingly. | ||
| * @param {number[]} pageSizeIncrements (optional) Page size increments for the 'per page' dropdown. If not |
There was a problem hiding this comment.
Just a question here... Do we aim to describe typed arrays?
There was a problem hiding this comment.
I don't see the harm in providing this info., just to let them know we're expecting an array of numbers. I can trim it to just 'Array' if you feel that's better.
There was a problem hiding this comment.
Fixed to: Array to match other ngDoc examples in a-pf repo.
src/pagination/pagination.html
Outdated
| <li ng-class="{'disabled': $ctrl.pageNumber === 1}"><a title="First Page" ng-click="$ctrl.gotoFirstPage()" class="goto-first-page"><span class="i fa fa-angle-double-left"></span></a></li> | ||
| <li ng-class="{'disabled': $ctrl.pageNumber === 1}"><a title="Previous Page" ng-click="$ctrl.gotoPreviousPage()" class="goto-prev-page"><span class="i fa fa-angle-left"></span></a></li> | ||
| </ul> | ||
| <input class="pagination-pf-page" type="text" ng-model="$ctrl.pageNumber" ng-change="$ctrl.onPageNumberChange()"/> |
There was a problem hiding this comment.
Appears the field lets you continue to a blank page if you manually enter pages that don't exist, maybe add a check to ignore higher than available pages.
There was a problem hiding this comment.
Fixed, defaults to last page if number > last page. Hmm..I should probably do the same for before first page (0, -1, etc).
There was a problem hiding this comment.
We may have inadvertently caused an additional issue, blanking out the field does this... NaN-NaN
There was a problem hiding this comment.
Hi, fixed issue, but now you have to enter the number and then hit Tab or Enter to apply the new page number.
| 'use strict'; | ||
| var ctrl = this; | ||
|
|
||
| var defaultPageSizeIncrements = [5, 10, 20, 40, 80, 100]; |
There was a problem hiding this comment.
Are we concerned about displaying large increments when the number of results is exceedingly low? Like say 2...
There was a problem hiding this comment.
I'm not quite following, if you only have two results/items, but a large pageSize, like 50, it would still only show one page with the two results.
There was a problem hiding this comment.
Ah yes, not so clear. Had to do with me reducing the number of table entries to "2" then still seeing the page results dropdown showing increments up to 100.
There was a problem hiding this comment.
Ok, yeah, still think it's valid to always show all the increments, regardless of results, keeps it more consistent. -thanks
src/pagination/pagination.service.js
Outdated
| }) | ||
| .filter('startFrom', function () { | ||
| return function (input, start) { | ||
| start = +start; //parse to int |
There was a problem hiding this comment.
Hadn't seen that shortcut in awhile, we could remove the explanation by using parseInt(start).
There was a problem hiding this comment.
Yeah, this was cut&paste code, I'll adjust it. -thanks
3 participants
Common pagination controls to be used with List, Card, and Table views
@akieling, @serenamarie125. @LHinson
JIRA: https://patternfly.atlassian.net/browse/OSUX-25