feat(pagination): add initial pagination component #5156

andrewseguin · 2017-06-16T00:00:25Z

jelbourn · 2017-06-16T00:33:17Z

src/lib/paginator/paginator.html

+</div>
+
+<button md-icon-button
+        class="mat-paginator-navigation-button mat-paginator-navigation-previous"


Looks like mat-paginator-navigation-button isn't used for anything?

Right, good catch thanks. This was there before adding explicit previous/next classes and isn't needed now.

jelbourn · 2017-06-16T00:38:48Z

src/lib/paginator/paginator-intl.ts

+
+/**
+ * Paginator labels that require internationalization. To modify the labels and text displayed,
+ * extend this class with custom values and inject it as a custom provider.


"Inject" isn't really the right word here, and extending isn't strictly necessary; I would just say something like

To modify the labels and text displayed, create a new instance of MdPaginatorIntl and include it in a custom provider

jelbourn · 2017-06-16T00:41:37Z

src/lib/paginator/paginator-intl.ts

+  previousPageLabel = 'Previous page';
+
+  /** A label for the range of items within the current page and the length of the whole list. */
+  getRangeLabel(currentPage: number, pageSize: number, listLength: number) {


Instead of making this a method, could just be a function property so that it can just be set

getRangeLabel = (page: number, pageSize: number, length: number) => { ... }

jelbourn · 2017-06-16T00:45:44Z

src/lib/paginator/paginator.md

@@ -0,0 +1,24 @@
+`<md-paginator>` is a component that provides navigation between paged information. It supports
+ the changing of page length as well as giving the user buttons to navigate to the previous or next
+ page.


Replace first paragraph with

`<md-paginator>` is a navigation for pages information, typically used with a data-table.

jelbourn · 2017-06-16T00:48:13Z

src/lib/paginator/paginator.ts

+  _pageLengthOptions: number[] = [];
+
+  /** Event emitted when the paginator changes the page length or current page index. */
+  @Output() pageChange = new EventEmitter<PageChangeEvent>();


Just page

jelbourn · 2017-06-16T00:54:02Z

src/lib/paginator/paginator.ts

+  }
+
+  /** Returns true if the user can go to the next page. */
+  canNavigateToPreviousPage() {


hasPreviousPage() and hasNextPage()?

jelbourn · 2017-06-16T00:58:53Z

src/lib/paginator/paginator.ts

+
+  /** The set of provided page length options to display to the user. */
+  @Input()
+  set pageLengthOptions(pageLengthOptions: number[]) {


pageSizeOptions

jelbourn · 2017-06-16T01:01:24Z

src/lib/paginator/paginator.md

+
+To use the paginator, you must provide the total length of the pagination information list and
+the length of each page. To allow the end-user to change the page length list, you may optionally
+input an array of numbers that the user may select from.


Replace this paragraph and the next:

### Basic use Each paginator instances requires: * The current page index * The number of items per page * The total number of items being paged When the user interacts with the paginator, a `PageEvent` will be fired that can be used to update any associated data view. ### Page size options The paginator displays a dropdown of page sizes for the user to choose from. The options for this dropdown can be set via `pageSizeOptions`

jelbourn · 2017-06-16T01:02:36Z

src/lib/paginator/paginator.md

+`PageChangeEvent` that contains the paginator's context, including the list length,
+
+### Internationalization
+In order to support internationalization or customization of the displayed text, the paginator


Can shorten to

The labels for the paginator can be customized by providing your own instance of `MdPaginatorIntl`

jelbourn · 2017-06-16T01:06:25Z

src/lib/paginator/paginator.ts

+  }
+
+  /** Emits an event notifying that a change of the paginator's properties has been triggered. */
+  private _sendPageEvent() {


_emitPageEvent

devversion

I think the paginator/index.ts path should be added to Dgeni (tools/dgeni/index.js)

devversion · 2017-06-16T08:29:41Z

src/lib/paginator/index.ts

+  ],
+  exports: [MdPaginator],
+  declarations: [MdPaginator],
+  providers: [MdPaginatorIntl],


Should this provider be treated as a singleton? Since we don't distinguish between forRoot() I think there would be multiple instances of this provider when lazy loading.

That's correct, it'll have multiple instances though that may be desired for the user if they want to customize their paginator not on internationalization but on the type of data. E.g. instead of "Items per page:" they can put "Users per page:".

@jelbourn what do you think

That's a good argument. Even though it kinda sounds confusing when the provider is called "Intl"

crisbeto · 2017-06-16T16:12:36Z

src/demo-app/data-table/person-data-source.ts

+
+    const streams = [collectionViewer.viewChange, this._displayData];
+    return Observable.combineLatest(streams).map((results: any[]) => {
+      const view: {start: number, end: number} = results[0];


You could reduce this one down to const [view, data] = results.

The downside is that I lose the typing of view, unless there's a syntax I'm missing that would let me apply different typing to view and data

I thought that TS might be smart enough to infer the type. It's not a big deal to leave it as it is.

Cool, got it working by changing the results from any[] to a tuple (results: [{start: number, end: number}, UserData[]]). It is smart enough to do const [view, data] = results

crisbeto · 2017-06-16T16:14:07Z

src/lib/core/testing/jasmine-matchers.ts


-        return result;
+  /** Matcher verifying that the element has the expected aria label. */
+  toHaveAriaLabel: () => {


Do we really need an extra matcher just for the aria-label? It seems like we can end up with having a matcher for every attribute.

Yeah not sure this ended up making things easier, and it doesn't scale. Tried implementing a generic "hasAttributes" matcher but still doesn't seem too useful. Taking it out

crisbeto · 2017-06-16T16:16:11Z

src/lib/paginator/paginator-intl.ts

+ * extend this class with custom values and inject it as a custom provider.
+ */
+@Injectable()
+export class MdPaginatorIntl {


Perhaps we should have an interface that the user can implement when overriding?

Perhaps, if we do then we should also look at changing MdDatepickerIntl. I followed along with what we did in that class

crisbeto · 2017-06-16T16:18:46Z

src/lib/paginator/paginator.html

+        [attr.aria-label]="_intl.previousPageLabel"
+        [mdTooltip]="_intl.previousPageLabel"
+        [mdTooltipPosition]="'above'"
+        [disabled]="!canNavigateToPreviousPage(-1)">


This one should be [disabled]="!canNavigateToPreviousPage(-1) || null", otherwise the disabled attribute will evaluate to disabled="false" which is still disabled. Same goes for the next button below.

Not sure I follow why || null is necessary.

Won't disabled="false" be evaluated as false since it is passed through coerceBooleanProperty which will check if it is the string false?

Edit: Should clarify, I believe this is handled by md-button

Oh, I didn't notice it was an md-button. Then it should work.

crisbeto · 2017-06-16T16:20:20Z

src/lib/paginator/paginator.html

+        [disabled]="!canNavigateToPreviousPage(-1)">
+  <div class="mat-paginator-increment"></div>
+</button>
+<button md-icon-button


This won't work in compatibility mode.

We added the following to the @Component:

providers: [ {provide: MATERIAL_COMPATIBILITY_MODE, useValue: false} ],

Which should make sure that the paginator's template will not be in compatibility mode.

crisbeto · 2017-06-16T16:26:30Z

src/lib/paginator/paginator.ts

+    this._updateDisplayedPageLengthOptions();
+  }
+  get pageLength(): number { return this._pageLength; }
+  _pageLength: number;


Shouldn't this be private? Same goes for all the other underscored properties.

crisbeto · 2017-06-16T16:28:46Z

src/lib/paginator/paginator.ts

+    }
+
+    // Sort the numbers using a number-specific sort function.
+    this._displayedPageLengthOptions.sort((a, b) => (a - b));


Unnecessary parentheses here: this._displayedPageLengthOptions.sort((a, b) => a - b);

crisbeto · 2017-06-16T16:30:55Z

src/lib/paginator/_paginator-theme.scss

+    border-right: 2px solid mat-color($foreground, 'icon');
+  }
+
+  .mat-icon-button[disabled] .mat-paginator-increment,


You can nest this to avoid repetition:

.mat-icon-button[disabled] { .mat-paginator-increment, .mat-paginator-decrement { border-color: mat-color($foreground, 'disabled'); } }

crisbeto · 2017-06-16T16:31:45Z

src/lib/paginator/paginator.html

+<button md-icon-button
+        class="mat-paginator-navigation-button mat-paginator-navigation-next"
+        (click)="navigateToNextPage()"
+        [attr.aria-label]="_intl.nextPageLabel"


Just [aria-label] should work.

Funny thing here, since that works for md-select but not md-button since md-select explicitly marks this as an @Input and handles applying the attribute. For the md-button however, Angular complains that it is not an input. In the case of aria attributes, they are not properties of the DOM element object and so cannot be applied with just [aria-label]

crisbeto · 2017-06-16T16:32:23Z

src/lib/paginator/paginator.scss

+.mat-paginator-decrement {
+  margin-left: 12px;
+  [dir='rtl'] & {
+    margin-right: 12px;


Shouldn't the margin-left be reset in RTL?

jelbourn · 2017-06-17T14:52:51Z

src/lib/paginator/paginator.md

+
+### Basic use
+Each paginator instance requires:
+* The current page index


Slight correction from my earlier comment, the page index isn't required now. So you can cut that line and then add below

"The current page index defaults to 0, but can be explicitly set via pageIndex."

jelbourn · 2017-06-17T14:53:52Z

src/lib/paginator/paginator.md

+
+### Page size options
+The paginator displays a dropdown of page sizes for the user to choose from. The options for this
+dropdown can be set via `pageSizeOptions`


Can add

"The current pageSize will always appear in the dropdown, even if it is not included in pageSizeOptions."

jelbourn · 2017-06-17T14:57:01Z

src/lib/paginator/paginator.html

@@ -0,0 +1,39 @@
+<div class="mat-paginator-page-length-select">


mat-paginator-page-size-selection

jelbourn · 2017-06-17T14:57:48Z

src/lib/paginator/paginator.scss

+  display: flex;
+  align-items: center;
+
+  .mat-select {


Use a class for this instead of doing a child selector

jelbourn · 2017-06-17T15:05:10Z

src/lib/paginator/paginator.ts

+
+  /** Number of items to display on a page. By default set to the first of page size option. */
+  @Input()
+  get pageSize(): number { return this._pageSize; }


Is there a default for these?

andrewseguin · 2017-06-19T19:04:13Z

Made the code changes, ready for review. The page size is currently set to default to 0. Let me know if you want that to be removed or increased to something like 10 or 25.

Note that the SCSS contains a nested class rule for .mat-select-trigger which I don't think I'll be able to get around.

crisbeto

LGTM, a couple of minor notes left.

crisbeto · 2017-06-19T19:46:18Z

src/lib/paginator/paginator.ts

+  /** Returns true if the user can go to the next page. */
+  hasNextPage() {
+    const numberOfPages = Math.ceil(this.length / this.pageSize) - 1;
+    return (this.pageIndex + 1) <= numberOfPages && this.pageSize != 0;


Hmm, shouldn't this.pageIndex < numberOfPages instead of (this.pageIndex + 1) <= numberOfPages technically be the same?

crisbeto · 2017-06-19T19:55:36Z

src/lib/paginator/paginator.ts

+
+  /** Returns true if the user can go to the next page. */
+  hasPreviousPage() {
+    return (this.pageIndex - 1) >= 0 && this.pageSize != 0;


The (this.pageIndex - 1) >= 0 could we rewritten to this.pageIndex >= 1.

crisbeto · 2017-06-19T19:56:18Z

src/material-examples/paginator-overview/paginator-overview-example.html

+<md-paginator [length]="100"
+              [pageSize]="10"
+              [pageSizeOptions]="[5, 10, 25, 100]" ]>
+</md-paginator>


This file is missing a newline at the end of the file.

andrewseguin · 2017-06-19T22:11:23Z

Thanks @crisbeto, those conditionals look better

ErinCoughlan · 2017-06-19T21:58:57Z

src/demo-app/data-table/person-data-source.ts

+          let rangeEnd = Math.min(data.length, view.end + buffer);
+
+          for (let i = rangeStart; i < rangeEnd; i++) {
+            this._renderedData[i] = data[i];


What about using slice instead of a for loop? I believe it's faster and in this case, about as intuitive.

benchmark (index vs slice)

This is because I'll want renderedData to be a virtual buffer that fills in over time. E.g. the table says it wants rows 0-10 and then the user scrolls to the end and asks for 90-100. Rendered data should be an array that contains 100 items that are all undefined except for 0-10 and 90-100

ErinCoughlan · 2017-06-19T22:02:53Z

src/lib/paginator/paginator-intl.ts

+@Injectable()
+export class MdPaginatorIntl {
+  /** A label for the page size selector. */
+  itemsPerPageLabel = 'Items per page:';


Would having these as inputs to the paginator with default values make more sense?

For my understanding of this class as well, if I only wanted to change the "Items per page" text, I can only override that one field?

When looking at this from an internationalization point of view, I think the injected class makes more sense (e.g. when a RTL language should be displayed). Though from a customization point of view, an @Input makes sense.

@jelbourn Paul and Erin both brought up some points about this class, any thoughts on renaming or changing?

From my POV, the most common thing I will want to do is set the i18n text so that "next page" becomes "Página siguiente" or whatever language the user is in.

I will never be able to use this default class, but for someone who just wants to render a table, it would be useful, although not sure if more useful than a default input. For RTL, we rely primarily on CSS, which you have covered.

We use injection for i18n because it's something that's defined once and doesn't need to be change-detected / have generated code.

ErinCoughlan · 2017-06-19T22:04:53Z

src/lib/paginator/paginator.scss

+  [dir='rtl'] & {
+    margin-right: $mat-paginator-button-increment-icon-margin;
+  }
+}


Missing newline at end of file

ErinCoughlan · 2017-06-19T22:06:34Z

src/lib/paginator/paginator.spec.ts

+      component.pageSize = 5;
+      component.pageIndex = 2;
+      fixture.detectChanges();
+      expect(rangeElement.innerText).toBe('11 - 15 of 10');


This seems wrong. Why would I only be looking at values greater than how many items are available? Same with the next expect as well.

Definitely is wrong =D but its wrong from the developer since they are telling us to go to a page that should not exist. Other alternative is to throw an error, but we opted to go the route of displaying what the developer told us to display (that is, items 11-15).

ErinCoughlan · 2017-06-19T22:10:28Z

src/lib/paginator/paginator.spec.ts

+        MdCommonModule,
+        MdSelectModule,
+        MdTooltipModule,
+        MdPaginatorModule,


Just include the MdPaginatorModule? The rest should be pulled in just from using it.

This is similar to what we did with table -- only include the single module so you're testing the way a user would use the component. This lets you identify if a module is missing.

Gah, made the same mistake twice. Thanks for pointing this out, much better to mimic what the users will do

ErinCoughlan · 2017-06-19T22:11:04Z

src/material-examples/paginator-configurable/paginator-configurable-example.html

+  <div>List length: {{pageEvent.length}}</div>
+  <div>Page size: {{pageEvent.pageSize}}</div>
+  <div>Page index: {{pageEvent.pageIndex}}</div>
+</div>


Missing newline.

jelbourn

LGTM

jelbourn · 2017-06-21T04:37:18Z

Caretaker note: will need build rules

ErinCoughlan

LGTM

tosehee75 · 2017-06-28T19:24:52Z

Is this tool available any time soon?

GustavoCostaW · 2017-07-07T03:05:23Z

Hey @andrewseguin and @jelbourn amazing job, congratz! The material team have an interest in pagination like this?

you can check my code in

https://github.com/GustavoCostaW/ngc-pagination

angular-automatic-lock-bot · 2019-09-07T20:01:50Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

andrewseguin added the pr: needs review label Jun 16, 2017

andrewseguin requested review from DerekLouie, jelbourn, ErinCoughlan, crisbeto and devversion June 16, 2017 00:00

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 16, 2017

andrewseguin force-pushed the pagination-component branch from 3dca91a to ab04bc7 Compare June 16, 2017 00:15

jelbourn reviewed Jun 16, 2017

View reviewed changes

devversion reviewed Jun 16, 2017

View reviewed changes

crisbeto reviewed Jun 16, 2017

View reviewed changes

andrewseguin force-pushed the pagination-component branch from a794230 to 53fe355 Compare June 16, 2017 18:02

jelbourn reviewed Jun 17, 2017

View reviewed changes

andrewseguin force-pushed the pagination-component branch from e5b28d0 to 2d43e39 Compare June 19, 2017 19:02

crisbeto approved these changes Jun 19, 2017

View reviewed changes

ErinCoughlan reviewed Jun 19, 2017

View reviewed changes

jelbourn approved these changes Jun 21, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed pr: needs review labels Jun 21, 2017

ErinCoughlan approved these changes Jun 21, 2017

View reviewed changes

andrewseguin added 5 commits June 23, 2017 10:30

check in

8fc6163

checkin

bcfbcd9

accessibility

666d7d5

checkin test

8181773

add examples and readme

2151297

andrewseguin added 14 commits June 23, 2017 10:36

fix lint

dd413cf

desc for getRangeLabel

f36ba2d

shorten inputs, fix scss, other comments

e273307

jsdoc

9819db8

intl change

c4cce4c

remove custom matcher

96ccd54

specify results type

8ae1ffc

lint

0246d1b

comments

3b6fb01

add default page size 50

e77d043

changes

7f57b5e

remove modules from spec

1a52793

remove fdescribe

746c3db

rebase

d52548a

andrewseguin force-pushed the pagination-component branch from 81e7ec6 to d52548a Compare June 23, 2017 17:44

jelbourn merged commit 85fb00a into angular:master Jun 23, 2017

andrewseguin deleted the pagination-component branch November 28, 2017 20:36

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

		@@ -0,0 +1,39 @@
		<div class="mat-paginator-page-length-select">

feat(pagination): add initial pagination component #5156

feat(pagination): add initial pagination component #5156

Conversation

andrewseguin commented Jun 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devversion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin Jun 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Jun 19, 2017

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn commented Jun 21, 2017

ErinCoughlan left a comment

Choose a reason for hiding this comment

tosehee75 commented Jun 28, 2017

GustavoCostaW commented Jul 7, 2017 • edited

angular-automatic-lock-bot bot commented Sep 7, 2019

andrewseguin commented Jun 16, 2017 •

edited

andrewseguin Jun 16, 2017 •

edited

GustavoCostaW commented Jul 7, 2017 •

edited