-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
feat(pagination): add initial pagination component #5156
Conversation
andrewseguin
commented
Jun 16, 2017
•
edited
Loading
edited
3dca91a
to
ab04bc7
Compare
src/lib/paginator/paginator.html
Outdated
</div> | ||
|
||
<button md-icon-button | ||
class="mat-paginator-navigation-button mat-paginator-navigation-previous" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like mat-paginator-navigation-button
isn't used for anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, good catch thanks. This was there before adding explicit previous/next classes and isn't needed now.
src/lib/paginator/paginator-intl.ts
Outdated
|
||
/** | ||
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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
src/lib/paginator/paginator-intl.ts
Outdated
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) => { ... }
src/lib/paginator/paginator.md
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace first paragraph with
`<md-paginator>` is a navigation for pages information, typically used with a data-table.
src/lib/paginator/paginator.ts
Outdated
_pageLengthOptions: number[] = []; | ||
|
||
/** Event emitted when the paginator changes the page length or current page index. */ | ||
@Output() pageChange = new EventEmitter<PageChangeEvent>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just page
src/lib/paginator/paginator.ts
Outdated
} | ||
|
||
/** Returns true if the user can go to the next page. */ | ||
canNavigateToPreviousPage() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasPreviousPage()
and hasNextPage()
?
src/lib/paginator/paginator.ts
Outdated
|
||
/** The set of provided page length options to display to the user. */ | ||
@Input() | ||
set pageLengthOptions(pageLengthOptions: number[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pageSizeOptions
src/lib/paginator/paginator.md
Outdated
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`
src/lib/paginator/paginator.md
Outdated
`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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can shorten to
The labels for the paginator can be customized by providing your own instance of `MdPaginatorIntl`
src/lib/paginator/paginator.ts
Outdated
} | ||
|
||
/** Emits an event notifying that a change of the paginator's properties has been triggered. */ | ||
private _sendPageEvent() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_emitPageEvent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the paginator/index.ts
path should be added to Dgeni (tools/dgeni/index.js
)
], | ||
exports: [MdPaginator], | ||
declarations: [MdPaginator], | ||
providers: [MdPaginatorIntl], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good argument. Even though it kinda sounds confusing when the provider is called "Intl"
|
||
const streams = [collectionViewer.viewChange, this._displayData]; | ||
return Observable.combineLatest(streams).map((results: any[]) => { | ||
const view: {start: number, end: number} = results[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could reduce this one down to const [view, data] = results
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that TS might be smart enough to infer the type. It's not a big deal to leave it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
return result; | ||
/** Matcher verifying that the element has the expected aria label. */ | ||
toHaveAriaLabel: () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/lib/paginator/paginator-intl.ts
Outdated
* extend this class with custom values and inject it as a custom provider. | ||
*/ | ||
@Injectable() | ||
export class MdPaginatorIntl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should have an interface that the user can implement when overriding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, if we do then we should also look at changing MdDatepickerIntl
. I followed along with what we did in that class
src/lib/paginator/paginator.html
Outdated
[attr.aria-label]="_intl.previousPageLabel" | ||
[mdTooltip]="_intl.previousPageLabel" | ||
[mdTooltipPosition]="'above'" | ||
[disabled]="!canNavigateToPreviousPage(-1)"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't notice it was an md-button. Then it should work.
src/lib/paginator/paginator.html
Outdated
[disabled]="!canNavigateToPreviousPage(-1)"> | ||
<div class="mat-paginator-increment"></div> | ||
</button> | ||
<button md-icon-button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work in compatibility mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/lib/paginator/paginator.ts
Outdated
this._updateDisplayedPageLengthOptions(); | ||
} | ||
get pageLength(): number { return this._pageLength; } | ||
_pageLength: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be private? Same goes for all the other underscored properties.
src/lib/paginator/paginator.ts
Outdated
} | ||
|
||
// Sort the numbers using a number-specific sort function. | ||
this._displayedPageLengthOptions.sort((a, b) => (a - b)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary parentheses here: this._displayedPageLengthOptions.sort((a, b) => a - b);
border-right: 2px solid mat-color($foreground, 'icon'); | ||
} | ||
|
||
.mat-icon-button[disabled] .mat-paginator-increment, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can nest this to avoid repetition:
.mat-icon-button[disabled] {
.mat-paginator-increment,
.mat-paginator-decrement {
border-color: mat-color($foreground, 'disabled');
}
}
src/lib/paginator/paginator.html
Outdated
<button md-icon-button | ||
class="mat-paginator-navigation-button mat-paginator-navigation-next" | ||
(click)="navigateToNextPage()" | ||
[attr.aria-label]="_intl.nextPageLabel" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just [aria-label]
should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
src/lib/paginator/paginator.scss
Outdated
.mat-paginator-decrement { | ||
margin-left: 12px; | ||
[dir='rtl'] & { | ||
margin-right: 12px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the margin-left
be reset in RTL?
a794230
to
53fe355
Compare
src/lib/paginator/paginator.md
Outdated
|
||
### Basic use | ||
Each paginator instance requires: | ||
* The current page index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
."
|
||
### 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` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can add
"The current pageSize
will always appear in the dropdown, even if it is not included in pageSizeOptions
."
src/lib/paginator/paginator.html
Outdated
@@ -0,0 +1,39 @@ | |||
<div class="mat-paginator-page-length-select"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mat-paginator-page-size-selection
src/lib/paginator/paginator.scss
Outdated
display: flex; | ||
align-items: center; | ||
|
||
.mat-select { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a class for this instead of doing a child selector
src/lib/paginator/paginator.ts
Outdated
|
||
/** 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; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a default for these?
e5b28d0
to
2d43e39
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a couple of minor notes left.
src/lib/paginator/paginator.ts
Outdated
/** 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, shouldn't this.pageIndex < numberOfPages
instead of (this.pageIndex + 1) <= numberOfPages
technically be the same?
src/lib/paginator/paginator.ts
Outdated
|
||
/** Returns true if the user can go to the next page. */ | ||
hasPreviousPage() { | ||
return (this.pageIndex - 1) >= 0 && this.pageSize != 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The (this.pageIndex - 1) >= 0
could we rewritten to this.pageIndex >= 1
.
<md-paginator [length]="100" | ||
[pageSize]="10" | ||
[pageSizeOptions]="[5, 10, 25, 100]" ]> | ||
</md-paginator> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is missing a newline at the end of the file.
Thanks @crisbeto, those conditionals look better |
let rangeEnd = Math.min(data.length, view.end + buffer); | ||
|
||
for (let i = rangeStart; i < rangeEnd; i++) { | ||
this._renderedData[i] = data[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using slice instead of a for loop? I believe it's faster and in this case, about as intuitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@Injectable() | ||
export class MdPaginatorIntl { | ||
/** A label for the page size selector. */ | ||
itemsPerPageLabel = 'Items per page:'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use injection for i18n because it's something that's defined once and doesn't need to be change-detected / have generated code.
[dir='rtl'] & { | ||
margin-right: $mat-paginator-button-increment-icon-margin; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline at end of file
component.pageSize = 5; | ||
component.pageIndex = 2; | ||
fixture.detectChanges(); | ||
expect(rangeElement.innerText).toBe('11 - 15 of 10'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
src/lib/paginator/paginator.spec.ts
Outdated
MdCommonModule, | ||
MdSelectModule, | ||
MdTooltipModule, | ||
MdPaginatorModule, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah, made the same mistake twice. Thanks for pointing this out, much better to mimic what the users will do
<div>List length: {{pageEvent.length}}</div> | ||
<div>Page size: {{pageEvent.pageSize}}</div> | ||
<div>Page index: {{pageEvent.pageIndex}}</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Caretaker note: will need build rules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
81e7ec6
to
d52548a
Compare
Is this tool available any time soon? |
Hey @andrewseguin and @jelbourn amazing job, congratz! The material team have an interest in pagination like this? you can check my code in |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |