fix(Pagination): handle max > 5 and max equal total pages

[Haythamasalama]

🔗 Linked issue

#723

❓ Type of change

• 🐞 Bug fix (a non-breaking change that fixes an issue)

📚 Description

• When the maximum value is set to 0, 1, 2, 3, or 4, the pagination display is not correct.

Before:	After:

Resolves #723

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
ui	✅ Ready (Inspect)	Visit Preview	Sep 28, 2023 10:33am

[Haythamasalama]

• Another use case is when the maximum displayed pages are equal to or greater than the total pages.

Before
After

[smarroufin]

Hi @Haythamasalama !

Thanks for the PR! Indeed there is no correct handling of max < 5. This is due to the fact that max is not intended to be set below 5 as it technically has no sens, based on its initial purpose. Let me explain what was thought in the first place 🙂

max has the goal to keep a max number of buttons (not only "pages" buttons but also "ellipsis" buttons), to keep consistency in its integration/display.
See how the number of buttons don't change on production:

And see how the changes affect this UX:

I'm fully open to rewrite that algorithm, I'm not really satisfied of what I've written initially and of how maintainable it is. But it should keep the initial behaviour, to allow some users to restrict the Pagination width and have a consistent UI.

Also about your PR, it looks good when page 1 is selected, but not fine when moving to page 2/3 in this case:

If you can fix that, we are good to go! 🚀

[Haythamasalama]

Thanks ❤️ @smarroufin for explaining the Max props. The first time, I thought max was only for buttons without dividers

However, after the explanation of what max is, I agree that it doesn't make sense for it to be max > 5 but we still need to handle cases when max > 5 as 5 and when max is equal to the total number of pages.

So, I made a refactor for it using the same algorithm to handle those cases.

[smarroufin]

LGTM with your last updates! Works very well!

Also, to be a bit picky, why wouldn't we remove (or update) the props.max validator with it?
I guess it doesn't make sens since we fallback to a valid value on our own now.

      validate (value) {
        return value >= 7 && value < Number.MAX_VALUE
      }

[Haythamasalama]

@smarroufin

I agree that we need to set the maximum value on the validator to 5, but should we remove the number from here?

                                               
 const maxDisplayedPages = Math.max(props.max, 5)

[smarroufin]

Do you mean 5 as minimum value? Then yes. From 5 to anything.

And I guess your fallback is to be kept anyways, otherwise we would end up with the issue we currently have, meaning that the docs number select can "disrespect" the validator by giving an unappropriated number.

[Haythamasalama]

Sure, I've updated to 5.

[smarroufin]

All good for me, thanks a lot @Haythamasalama!

[Haythamasalama]

All good for me, thanks a lot @Haythamasalama!

You're welcome, @smarroufin ❤️. Thank you for reviewing it.