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

Fix pagination issues #273

Merged
merged 6 commits into from
Oct 6, 2021
Merged

Fix pagination issues #273

merged 6 commits into from
Oct 6, 2021

Conversation

codemonkey800
Copy link
Collaborator

@codemonkey800 codemonkey800 commented Oct 5, 2021

Description

Fixes issues found during QA:

  1. when user clicks the next/back page... page remains at the bottom when it should go up every time user navigates the pagination
  2. when page 1 or last page if displayed, if user positions the mouse next to the pagination, there is a hover state that should not be showing up
  3. if there is only 5 pages, and in the search bar user enters page 7 for example, first page is displayed, instead of last page, in this case. page 5

Demos

https://pagination-frontend.dev.imaging.cziscience.com/

Pagination scrolls to top when changing pages

Before the page would stay at the bottom when navigating.

scroll-on-page-change.mp4

Remove hover state from disabled buttons

Before

highlight-state-before

After

highlight-state-after

Open closest extreme for out-of-range pages

Before, the page would always start at page 1 if the page was out of range. Now, the page will be set to the closest extreme:

  1. If the page is < 1 or not a number, then the page is automatically set to first page.
  2. If the page is > totalPages, the page is set to the last page.

page-invalid-values

: page;
// If the page number is not a number or not in range, then set it to the
// beginning or ending page depending on which is closer.
if (Number.isNaN(page) || page < BEGINNING_PAGE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if page > RESULTS_PER_PAGE - 1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm curious how RESULTS_PER_PAGE is related 🤔 The constant RESULTS_PER_PAGE refers to how many search results are on a page.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I referred to the wrong variable. I mean to ask page < BEGINNING_PAGE is checked on the left side of the range for page number, but how are we checking it on the right side of the range?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it should be the condition below page > totalPages for checking the right side of the range 😄

Copy link
Contributor

@justinelarsen justinelarsen left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for getting in these fixes so quickly! 🎉

@codemonkey800 codemonkey800 merged commit 1fdaf8f into main Oct 6, 2021
@codemonkey800 codemonkey800 deleted the jeremy/fix-pagination-issues branch October 6, 2021 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants