[Pagination] unexpected behavior when setting boundaryCount and siblingCount

[jhumpohl]

• [x] The issue is present in the latest release.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

1.) When i set the boundaryCount={0} just to show the current page and use the prev and next buttons. The current page is just displayed on the first page or last but isnt displayed when you navigate to any other page.

2.) When i set the boundaryCount={1} and the siblingCount={1} the pages that are shown add up. a) For example when the current page is on 5 from 10 The shown pages are < 1 ... 4 [5] 6 ... 10 > thats right. b) When you are at page 4 it looks like this < 1 2 3 [4] 5 ... 10 > and should be < 1 ... 3 [4] 5 ...>. c) When you are at page 1 it looks like this < [1] 2 3 4 5 ... 10 > and from my opinion should be like this < [1] 2 ... 10 >

Expected Behavior 🤔

1.) Show the current page any time :).

2.) a) is ok how it is b) should be < 1 ... 3 [4] 5 ...> c) should be < [1] 2 ... 10 >

Steps to Reproduce 🕹

I used the code Sandbox from the Examples and just changed the Props on the component.

https://codesandbox.io/s/bn7nw and added the props like described above. For 1) boundaryCount={0} For 2) boundaryCount={1} and the siblingCount={1}

Context 🔦

Its a Navigation for a Responsive Website with Page Results that need Filters,Limits and navigation. Because its for Phones too it needs to be small sometimes.

Your Environment 🌎

`npx @material-ui/envinfo`

``` Don't forget to mention which browser you used. Output from `npx @material-ui/envinfo` goes here. ```

[mbrookes]

This is by design, as it keeps the width of the pagination component stable.

[oliviertassinari]

@mbrookes I agree with point 2), the request is invalid, we should keep pagination width stable. However, on point 1, this

<Pagination boundaryCount={0} siblingCount={0} count={10} page={page} onChange={handleChange} />

seems to behave strangely https://codesandbox.io/s/material-demo-forked-2nkvf?file=/demo.js.

[oliviertassinari]

For instance, on page 7, we get:

why not the following?

or even

[jhumpohl]

I dont think the component keeps it width when you go up to a page count above 100 but if this is the reason i can understand point 2). I would maybe had drop the three points to keep the size.

But at point 1) its the same Reason? If so it makes no sense to me. the current page is much more important than the point or page count. I would had expected a solution like oliviertassinari posted.

But anyway thx for the quick awnser and support.

[oliviertassinari]

So, I had a look at the issue. It turns out that we have already fixed most of the issue in v5: https://codesandbox.io/s/material-demo-forked-2nkvf?file=/demo.js.

Having a second thought about it, I would propose that for:

<Pagination boundaryCount={0} siblingCount={0} count={10}

It renders:

Instead of:

It allows to have a denser display, the dots don't really add much. I could make it work with this diff:

index 4f15278cb1..f92c0bf84f 100644
--- a/packages/material-ui/src/usePagination/usePagination.js
+++ b/packages/material-ui/src/usePagination/usePagination.js
@@ -65,33 +65,41 @@ export default function usePagination(props = {}) {
     endPages.length > 0 ? endPages[0] - 2 : count - 1,
   );

-  // Basic list of items to render
-  // e.g. itemList = ['first', 'previous', 1, 'ellipsis', 4, 5, 6, 'ellipsis', 10, 'next', 'last']
-  const itemList = [
-    ...(showFirstButton ? ['first'] : []),
-    ...(hidePrevButton ? [] : ['previous']),
-    ...startPages,
-
-    // Start ellipsis
+  // Start ellipsis
+  const startEllipsis =
     // eslint-disable-next-line no-nested-ternary
-    ...(siblingsStart > boundaryCount + 2
+    siblingsStart > boundaryCount + 2
       ? ['start-ellipsis']
       : boundaryCount + 1 < count - boundaryCount
       ? [boundaryCount + 1]
-      : []),
-
-    // Sibling pages
-    ...range(siblingsStart, siblingsEnd),
+      : [];

-    // End ellipsis
+  // End ellipsis
+  const endEllipsis =
     // eslint-disable-next-line no-nested-ternary
-    ...(siblingsEnd < count - boundaryCount - 1
+    siblingsEnd < count - boundaryCount - 1
       ? ['end-ellipsis']
       : count - boundaryCount > boundaryCount
       ? [count - boundaryCount]
-      : []),
+      : [];

-    ...endPages,
+  const body =
+    boundaryCount === 0 && siblingCount === 0
+      ? [page]
+      : [
+          ...startPages,
+          ...startEllipsis,
+          ...range(siblingsStart, siblingsEnd),
+          ...endEllipsis,
+          ...endPages,
+        ];
+
+  // Basic list of items to render
+  // e.g. itemList = ['first', 'previous', 1, 'start-ellipsis', 4, 5, 6, 'end-ellipsis', 10, 'next', 'last']
+  const itemList = [
+    ...(showFirstButton ? ['first'] : []),
+    ...(hidePrevButton ? [] : ['previous']),
+    ...body,
     ...(hideNextButton ? [] : ['next']),
     ...(showLastButton ? ['last'] : []),
   ];

Should we move forward with it?

[jhumpohl]

To me first and last page is more important then the dots. And on small displays you dont really have the space. To me it looks good. The biggest issue was the missing of the current page in the current V4 version.

Nice job!

[fayzzzm]

@oliviertassinari, I know it could be a stupid question from me, but what if the developer passes count float value

<Pagination count={1.03} />

and next button won't be disabled due to this line of code

disabled:
            disabled ||
            (item.indexOf('ellipsis') === -1 &&
              (item === 'next' || item === 'last' ? page >= count : page <= 1))

Pagination code link

What if instead of checking page >= count make page >= parseInt(count) or to parseInt it at the beginning.

[oliviertassinari]

@fayzzzm What's the link with this issue? I don't think that the core logic should care about float. However, we could introduce a custom integer prop-type, we have a couple of other cases where PropTypes.number isn't ideal. If you want to work on it, contribution welcome 👍

[fayzzzm]

Ok, I will create an issue and you can have a look at it. We could discuss props validating after if it's ok?

[oliviertassinari]

@fayzzzm To be clear, we don't want to support count={1.03}, we want to throw a warning when developers try to us this API (at least, it seems to be what would make the most sense).

[fayzzzm]

@oliviertassinari Got you!