Skip to content

pfTableView with pfPagination #527

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 31, 2017
Merged

pfTableView with pfPagination #527

merged 2 commits into from
Jul 31, 2017

Conversation

dtaylor113
Copy link
Member

@dtaylor113 dtaylor113 commented Jul 26, 2017 •
edited
Loading

Integration of pfPagination controls into pfTableView

  • Forked angular-datatables due to issue. Angular-datatables has moved on to supporting angular 2 & 4, not sure if they will ever get around to addressing this issue in angularjs.
  • Former method of specifying dtOptions for pagination supported for backwards compatibility
  • Tested in pfTableView - Basic, pfTable, pfTableView - w/ Toolbar, and pfToolbar examples
  • Tested with Filtering, Sorting, Select All, Empty State, show/hide checkboxes, and Action buttons/menus

image

Updated ngDocs:
image

@akieling

@jeff-phillips-18
Copy link
Member

The pagination bar doesn't seem to right align correctly. Though, neither does the toolbar.

jeff-phillips-18
jeff-phillips-18 requested changes Jul 26, 2017
View reviewed changes
test/table/tableview/table-view.spec.js
expect(angular.element(element.find('.pagination-pf-items-total')).text().trim()).toBe('7');
expect(angular.element(element.find('.pagination-pf-page')).val().trim()).toBe('2');
expect(angular.element(element.find('.pagination-pf-pages')).text().trim()).toBe('4');
});
Copy link
Member

Choose a reason for hiding this comment

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

Please add tests to make sure updatePageSize and updatePageNumber functions are called correctly when appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, do you mean with spy functions, or test changing the pageSize and pageNumber controls and getting the expected HTML?

Copy link
Member

Choose a reason for hiding this comment

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

Spy functions work, or just by saving the parameters and checking the values after. Similar to

https://github.com/patternfly/angular-patternfly/blob/master/test/notification/notification-drawer.spec.js#L332-L337

and

https://github.com/patternfly/angular-patternfly/blob/master/test/notification/notification-drawer.spec.js#L382-L389

Copy link
Member Author

Choose a reason for hiding this comment

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

Did both, fixed

src/table/table.less Outdated
@@ -27,6 +27,11 @@ table.dataTable {
}
}
tbody {
tr.selected {
span a {
color: white;
Copy link
Member

Choose a reason for hiding this comment

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

use @color-pf-white

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

src/pagination/pagination.component.js Outdated
@@ -131,11 +146,22 @@ angular.module('patternfly.pagination').component('pfPagination', {

ctrl.getEndIndex = function () {
var numFullPages = Math.floor(ctrl.numTotalItems / ctrl.pageSize);
var numItemsOnLastPage = ctrl.numTotalItems - (numFullPages * ctrl.pageSize);
var numItemsOnLastPage = ctrl.numTotalItems - (numFullPages * ctrl.pageSize) || ctrl.pageSize;
var numItemsOnPage = isLastPage() ? numItemsOnLastPage : ctrl.pageSize;
return ctrl.getStartIndex() + numItemsOnPage - 1;
Copy link
Member

Choose a reason for hiding this comment

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

If there are no items, this value would incorrectly be pageSize - 1.

Copy link
Member Author

@dtaylor113 dtaylor113 Jul 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed, although when I put items = [], I didn't get -1, I got '1-10' for an empty first page. Now it reads "0-0".

@dtaylor113
Copy link
Member Author

dtaylor113 commented Jul 27, 2017 •
edited
Loading

Hi @jeff-phillips-18, @cdcabrera, Does this PR warrant a Major version change since it has breaking changes? pfTableView won't work after this PR until end-users update their <scripts> to point to "angularjs-datatables" from "angular-datatables".

@cdcabrera
Copy link
Member

cdcabrera commented Jul 27, 2017 •
edited
Loading

@dtaylor113 I would concur, breaking change equals major increment

Edit: @dtaylor113 are we going to need to release package this change immediately, or is it something we can package with bug fixes?

@dtaylor113
Copy link
Member Author

are we going to need to release package this change immediately, or is it something we can package with bug fixes?

@cdcabrera This is an enhancement which OpenShift will "eventually" need, but it is not immediate. @serenamarie125 do you know when OpenShift will want pfTableView w/ pagination?

@jeff-phillips-18
Copy link
Member

I’d say keep it to a minor release. It won’t be worse than now if the application doesn't update the dependency, it will just use the old dependency and still have the bug. If the application is using wiredep, then all should be good.

@dtaylor113
Copy link
Member Author

dtaylor113 commented Jul 28, 2017 •
edited
Loading

I’d say keep it to a minor release.

So, v4.3.0 ? (current is v4.2.0)

@dtaylor113
Copy link
Member Author

Hi, so can this be merged?

@cdcabrera
Copy link
Member

lgtm

@cdcabrera cdcabrera merged commit 0ea5acb into patternfly:master Jul 31, 2017
@dtaylor113 dtaylor113 deleted the tableViewPages branch October 19, 2017 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdcabrera cdcabrera cdcabrera approved these changes

@jeff-phillips-18 jeff-phillips-18 jeff-phillips-18 approved these changes

@cshinn cshinn Awaiting requested review from cshinn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dtaylor113 @jeff-phillips-18 @cdcabrera