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

[Bug]: Adjacent ellipses in PaginationNav #15830

Closed
2 tasks done
cmarcink opened this issue Feb 27, 2024 · 7 comments
Closed
2 tasks done

[Bug]: Adjacent ellipses in PaginationNav #15830

cmarcink opened this issue Feb 27, 2024 · 7 comments

Comments

@cmarcink
Copy link

cmarcink commented Feb 27, 2024

Package

@carbon/react

Browser

Chrome, Firefox

Package version

v1.50.0

React version

v17.0.2

Description

Should not have adjacent ellipses in pagination.
image

Reproduction/example

https://react.carbondesignsystem.com/?path=/story/components-paginationnav--playground&args=itemsShown:4;totalItems:10

Steps to reproduce

  1. Render with props itemsShown = 4, totalItems = something >= 10
  2. Select a page from the ellipses
  3. Select the first page
  4. Notice the adjacent ellipses

image

Suggested Severity

None

Application/PAL

Cloud Pak for AIOps

Code of Conduct

@tay1orjones
Copy link
Member

tay1orjones commented Mar 4, 2024

Thanks for reporting, can confirm this looks off. I think something's wrong within this logic here:

// render first item if at least 5 items can be displayed or
// 4 items can be displayed and the current page is either 0 or 1
(itemsDisplayedOnPage >= 5 ||
(itemsDisplayedOnPage <= 4 && currentPage <= 1)) && (
<PaginationItem
page={1}
translateWithId={t}
isActive={currentPage === 0}
onClick={() => {
jumpToItem(0);
}}
/>
)

@tay1orjones
Copy link
Member

It's clear we don't want the overflows next to one another, but this brings up an interesting question for me: what do we want to happen? Should overflows be limited in certain cases beyond what we already have? What should we be aiming for when we fix this?

@carbon-design-system/design do you have any thoughts?

@hollyos
Copy link
Contributor

hollyos commented Apr 1, 2024

@tay1orjones It looks like this might be a bug with how the PaginationNav page selectors are working. The double ellipsis only shows when you navigate to page 1 from a page in the middle of the range (only when there are 2 ellipsis around the number). If you use the arrows to go up and down, the UI updates correctly to have 1 and 2 showing when 1 is selected.

Screenshot 2024-04-01 at 11 04 39

That said, I noticed that the UI should be more uniform in terms of which pages are visible for each active page, regardless of selection method. For example, when clicking on page 10 while page 1 is active, the page changes to 10, but 1 & 2 remain visible. Meanwhile, 8, 9, and 10 are visible with the arrows.

Page 1 Active

Screenshot 2024-04-01 at 10 43 00 Screenshot 2024-04-01 at 10 43 08

Page 10 Active

Screenshot 2024-04-01 at 10 42 52 Screenshot 2024-04-01 at 10 43 15

Seems the solution here would be to get the page list visually consistent for visible pages regardless of the selection method for each active page.


Outside of that fix, I have a couple of suggestions that could be implemented in a few steps.

  1. Change itemsShown to only target the number nav items. Then, update the minimum allowed value to 3.

    This is a potentially visually breaking change for some users' UIs. It would add 2 new items to current UIs in some cases.

    3

    active page example
    1 1 2 ... 10
    2 1 2 ... 10
    3 1 ... 3 ... 10
    9 1 ... 9 10
    10 1 ... 9 10

    4

    active page example
    1 1 2 3 ... 10
    2 1 2 3 ... 10
    3 1 ... 3 4 ... 10
    7 1 ... 7 8 ... 10
    8 1 ... 8 9 10
    10 1 ... 8 9 10

    * Examples here have been updated to incorporate the below changes. The boundary numbers in the examples above do not represent the current implementation.

  2. Add 2 new props; boundaryCount & siblingCount

    prop description
    boundaryCount Number of pages visible at the beginning and end.
    siblingCount Number of pages visible before and after the current page.

    * These should be implemented in separate PRs.

    These would provide more customization in the presentation of the pagination, allowing for control over which pages get displayed before, inside, and after the ellipsis.

@janhassel
Copy link
Member

When my design team was originally working on this we prototyped a few approaches, including the one outlined by @hollyos.

What we noticed during internal user testing was that it is extremely distracting / confusing to users when the width of the component changes while flipping through the pages. It also defeated the reason why we needed such a compact component since if there is enough space to show another item, why not show it initially.

I'm referring to these examples:

◂ 1 2 … 9 ▸
◂ 1 … 3 … 9 ▸
◂ 1 … 8 9 ▸
◂ 1 2 3 … 9 ▸
◂ 1 … 3 4 … 9 ▸
◂ 1 … 7 8 9 ▸

That's why we opted to include the overflows in the number of items shown when contributing the implementation.

@laurenmrice
Copy link
Member

I feel like Holly and Jan's approach for this makes sense, to try to keep the page list visually consistent for visible pages regardless of the selection method for each active page, and this solution would resolve the problem of having the double ellipsis appearing next to each other in the pagination.

Adding this as props is also a good idea, so people can use this if they need more control for their pagination needs.

@thyhmdo
Copy link
Member

thyhmdo commented Oct 1, 2024

Please review the design specs here for this fix

@thyhmdo thyhmdo moved this from ⏱ Backlog to 🚦 In Review in Design System Oct 8, 2024
@riddhybansal
Copy link
Contributor

Completed by #17628

@github-project-automation github-project-automation bot moved this from 🚦 In Review to ✅ Done in Design System Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

No branches or pull requests

8 participants