Skip to content
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

[Paginator] Add functionality to jump to first and last page #9278

Closed
Chris2011 opened this issue Jan 8, 2018 · 31 comments · Fixed by #9603
Closed

[Paginator] Add functionality to jump to first and last page #9278

Chris2011 opened this issue Jan 8, 2018 · 31 comments · Fixed by #9603
Assignees
Labels
feature This issue represents a new feature or feature request rather than a bug or bug fix help wanted The team would appreciate a PR from the community to address this issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@Chris2011
Copy link

Chris2011 commented Jan 8, 2018

Bug, feature request, or proposal:

Feature request

What is the expected behavior?

There should be 2 buttons like << and >> to jump to the first and last page.

What is the current behavior?

There is no button for this. There is only a button for < and > prev and next.

Items per page: 10 - 1 - 10 of 100 < >

What is the use-case or motivation for changing an existing behavior?

If you have a long list, often it is needed to jump to the end and back to the first page/first items.

Items per page: 10 - 1 - 10 of 100 << < > >>

Is there anything else we should know?

#7615 - as a reference.

@andrewseguin andrewseguin self-assigned this Jan 9, 2018
@andrewseguin andrewseguin added the feature This issue represents a new feature or feature request rather than a bug or bug fix label Jan 9, 2018
@andrewseguin andrewseguin added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent help wanted The team would appreciate a PR from the community to address this issue labels Jan 23, 2018
@lharries
Copy link
Contributor

Hi @andrewseguin I'd be keen to give this one a go

@andrewseguin
Copy link
Contributor

Sounds good to me, let's set the requirements and iterate from there:

@lharries
Copy link
Contributor

I agree with the above requirements.

For tests would it just be the spec tests? I don't see the paginator in the e2e tests.

@andrewseguin
Copy link
Contributor

Yup just some spec unit tests would suffice, try to consider the main case and edge cases, e.g. the jump-to-first button should be disabled when on the first page

@lharries
Copy link
Contributor

lharries commented Jan 24, 2018

Whilst doing the above I found two bugs in the spec file. Would it be best to do them as separate PRs or in the same PR that I'm currently working on, when I have completed this feature?

  1. A test was wrongly named. See fix here
  2. There was an off-by-one error where the index of the last page was wrongly calculated.
    This happened in both the goToLastPage() method and in the following test 'should disable navigating to the next page if at last page'. See fix here
    In headless chrome, it was going to page index 10 to check that it could no longer navigate forward. Whilst there are 10 pages they are starting from index 0 and therefore the last page is in fact index 9.

The bug fix for 2 is required in order to write a valid test for the current feature.

@andrewseguin
Copy link
Contributor

Good catch - if you submit the fixes via separate PRs, that'd be a lot easier for us.

@lharries
Copy link
Contributor

Thanks! And will do

@lharries
Copy link
Contributor

lharries commented Jan 24, 2018

Do I need to create issues for them or straight to the PRs?

@jelbourn jelbourn added help wanted The team would appreciate a PR from the community to address this issue and removed help wanted The team would appreciate a PR from the community to address this issue labels Jan 24, 2018
@andrewseguin
Copy link
Contributor

No issue needed, just a PR will do

@lharries
Copy link
Contributor

lharries commented Jan 24, 2018

Will do.

On a side note: for the Component attribute of showing/hiding the first and last buttons would you have any recommendations on naming? I am thinking hideFirstLastButtons, which would be similar to the current hidePageSize?

e.g.

<mat-paginator [pageIndex]="pageIndex"
                   [pageSize]="pageSize"
                   [pageSizeOptions]="pageSizeOptions"
                   [hidePageSize]="hidePageSize"
                   [hideFirstLastButtons]="hideFirstLastButtons"
                   [length]="length"
                   (page)="latestPageEvent = $event">
</mat-paginator>

@andrewseguin
Copy link
Contributor

Naming tends to be one of the harder parts of these sort of features.

By default, the paginator shouldn't show the first/last buttons anyways since the material spec doesn't include them. I imagine the flag would be more like showFirstAndLastButtons.

Curious how the interaction would look if you had a paginator for an "infinitely" long list. You'd want to make sure you can easily hide the Jump to Last button since it doesn't make sense.

@lharries
Copy link
Contributor

Ok, I'll make the first/last buttons hidden by default and implement the showFirstAndLastButtons attribute.

Shall I add showFirstButton and showLastButton too to allow the control such as with the "infinitely" long list?

@andrewseguin
Copy link
Contributor

Since its a pretty uncommon case and I'd lean more towards keeping a thinner API surface, let's leave those off. In that case it should be fairly trivial to just add a display: none to the last button.

If it comes up from users in the future, we can always add it in then

@lharries
Copy link
Contributor

Great point

lharries pushed a commit to lharries/material2 that referenced this issue Jan 25, 2018
the test was referencing the wrong page

discovered in angular#9278
@lharries
Copy link
Contributor

Is there a best practice for doing multiple PRs for the same file to avoid the conflicts on merge?

lharries pushed a commit to lharries/material2 that referenced this issue Jan 25, 2018
Whilst there are 10 pages they are starting from index 0 and therefore the last page is in fact index 9.
This error was repeated both in the length calculation and in the assertion statements, therefore the
test passed when it should have failed.

Discovered in angular#9278
@lharries
Copy link
Contributor

lharries commented Jan 25, 2018

I have added the first and last buttons to the demo app, in a new table, and to the material-examples, to the existing table, is this correct or would there be a better way to do this.

Additionally, would we need to submit a PR to the material.angular.io docs?

Here's where I'm currently at:

I'll have a look into the icons tomorrow but the functionality and all the tests are done 😄

lharries pushed a commit to lharries/material2 that referenced this issue Jan 25, 2018
Fix such that the previous page navigation button contains an icon with decrement,
and the next page navigation button contains an icon with increment. Previously
they were swapped around.

Noticed during fix for angular#9278
lharries pushed a commit to lharries/material2 that referenced this issue Jan 25, 2018
Fix such that the previous page navigation button contains an icon with class decrement,
and the next page navigation button contains an icon with class increment. Previously
they were incorrectly swapped.

Noticed during fix for angular#9278
lharries pushed a commit to lharries/material2 that referenced this issue Jan 25, 2018
Fix such that the previous page navigation button contains an icon with class decrement,
and the next page navigation button contains an icon with class increment. Previously
they were incorrectly swapped.

Noticed during fix for angular#9278
lharries pushed a commit to lharries/material2 that referenced this issue Jan 25, 2018
Add two buttons which allow you to jump to the first and last page.
Hidden by default and can be toggled with showHideFirstButtons

Fixes #angular#9278
@andrewseguin
Copy link
Contributor

Ah right - go ahead and include the fixes in your PR and then write in the description that they include fixes from the other PRs and that the PR is blocked until the others are in. Since they are small I imagine they will be merged fairly quickly. Once they are in, you can rebase and remove the disclaimer on your final PR

lharries pushed a commit to lharries/material2 that referenced this issue Jan 25, 2018
Add two buttons which allow you to jump to the first and last page.
Hidden by default and can be toggled with showHideFirstButtons

Fixes angular#9278
lharries pushed a commit to lharries/material2 that referenced this issue Jan 25, 2018
Whilst there are 10 pages they are starting from index 0 and therefore the last page is in fact index 9.
This error was repeated both in the length calculation and in the assertion statements, therefore the
test passed when it should have failed.

Discovered in angular#9278
@lharries
Copy link
Contributor

Thanks Andrew, will do!

@willshowell
Copy link
Contributor

This is awesome. Great work @lharries!

jelbourn pushed a commit that referenced this issue Jan 25, 2018
Whilst there are 10 pages they are starting from index 0 and therefore the last page is in fact index 9.
This error was repeated both in the length calculation and in the assertion statements, therefore the
test passed when it should have failed.

Discovered in #9278
jelbourn pushed a commit that referenced this issue Jan 25, 2018
the test was referencing the wrong page

discovered in #9278
lharries pushed a commit to lharries/material2 that referenced this issue Jan 26, 2018
Add two buttons which allow you to jump to the first and last page.
Hidden by default and can be toggled with showHideFirstButtons

Fixes angular#9278
lharries pushed a commit to lharries/material2 that referenced this issue Jan 27, 2018
Add two buttons which allow you to jump to the first and last page.
Hidden by default and can be toggled with showHideFirstButtons

Fixes angular#9278
lharries pushed a commit to lharries/material2 that referenced this issue Jan 27, 2018
Add two buttons which allow you to jump to the first and last page.
Hidden by default and can be toggled with showHideFirstButtons

Fixes angular#9278
tinayuangao pushed a commit that referenced this issue Jan 31, 2018
)

Add two buttons which allow you to jump to the first and last page.
Hidden by default and can be toggled with showHideFirstButtons

Fixes #9278
lharries pushed a commit to lharries/material2 that referenced this issue Jan 31, 2018
Fix such that the previous page navigation button contains an icon with class decrement,
and the next page navigation button contains an icon with class increment. Previously
they were incorrectly swapped.

Noticed during fix for angular#9278
lharries pushed a commit to lharries/material2 that referenced this issue Jan 31, 2018
Fix such that the previous page navigation button contains an icon with class decrement,
and the next page navigation button contains an icon with class increment. Previously
they were incorrectly swapped.

Noticed during fix for angular#9278
lharries pushed a commit to lharries/material2 that referenced this issue Jan 31, 2018
Fix such that the previous page navigation button contains an icon with class decrement,
and the next page navigation button contains an icon with class increment. Previously
they were incorrectly swapped.

Noticed during fix for angular#9278
lharries pushed a commit to lharries/material2 that referenced this issue Jan 31, 2018
Fix such that the previous page navigation button contains an icon with class decrement,
and the next page navigation button contains an icon with class increment. Previously
they were incorrectly swapped.

Noticed during fix for angular#9278
@btroncone
Copy link

@lharries I'm seeing this in Edge on first render:

image

Once you click on the button it corrects:

image

Any ideas?

@lharries
Copy link
Contributor

@btroncone apologies for that. I'll set up an Edge development environment and get this fixed

@btroncone
Copy link

@lharries No worries! Had QA catch it in one of my projects and wanted to make you aware. I messed around with it briefly but didn't reach a solution I was happy with, hopefully you will have better luck!

@lharries
Copy link
Contributor

@btroncone I'll create an issue and see if we can get it fixed

@lharries
Copy link
Contributor

lharries commented Feb 13, 2018

@btroncone there's a fix already submitted #9776

@btroncone
Copy link

@lharries Awesome, thanks for the update!

@tomliem
Copy link

tomliem commented Apr 15, 2019

Hi how to install this package @lharries ? is it by the default npm install --save @angular/material?

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature This issue represents a new feature or feature request rather than a bug or bug fix help wanted The team would appreciate a PR from the community to address this issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants