Improve API clarity for the PaginationRoot component

[MellKam]

Link to minimal reproduction

https://www.radix-vue.com/components/pagination#pagination

Describe the bug

When you pass 100 to the total props, it shows a maximum of 10 items. When you make 1000, it shows a maximum of 100. Essentially, it is divisible by 10, but it shouldn't be that way. You can check the code example on the website where total is set to 100, but it shows only 10 items.

[epr3]

Could you provide more context on this bug?
total refers to the number of items that would require pagination, therefore the number of pages displayed would be Math.ceil(total / itemsPerPage), where itemsPerPage has the default value of 10.

Thanks!

[MellKam]

@epr3 Oh, sorry. My bad. I just thought that it means the total number of pages..

Maybe setting default value for itemsPerPage is not completelly correct? Because it doesn't force you to think about your page size. And the page size is important, it will vary almost in any usecase.

Wait, why do we even need to specify the page size? I think this isn't necessary information for pagination. I tool a look at how other ui libraries do pagination, and in most cases they don't specify page size in props, only total number of pages. For example:

https://mui.com/material-ui/react-pagination/
https://nextui.org/docs/components/pagination
https://vuetifyjs.com/en/components/paginations/

What do you think?

[epr3]

I think that itemsPerPage covers the use case where you would have to paginate a Data Table. E.g. https://www.shadcn-vue.com/docs/components/data-table.html#pagination-1

I guess we could set itemsPerPage to 1 so that the default Pagination resembles the examples you provided.

[zernonia]

I believe the docs is pretty clear right? 😅

Setting itemsPerPage to default 1 is bad as it kinda defeat the purpose of "pagination"

[MellKam]

@zernonia Yeah, I get it. But still, the API feels a bit misleading. This code doesn't tell me that I will actually get 10 pages, instead of 100. I think that API should teach me how to use it, not the docs.

<PaginationRoot
  :total="100"
  :sibling-count="1"
  show-edges
  :default-page="2"
>
	...
</PaginationRoot>

I would at least add the itemsPerPage props to the example code on website to explain why it works this way. But much better would be to remove default value and make itemsPerPage required, so that we are forced to think about it. It's not a big deal, but it will make the API more clear.

<PaginationRoot
  :total="100"
  :items-per-page="10"
  :sibling-count="1"
  show-edges
  :default-page="2"
>
	...
</PaginationRoot>

If we dig even more, the number of items on a page is not really necessary to render pagination component. It is only used in the PaginationRoot to calculate the pageCount, which is indeed what we need.

https://github.com/radix-vue/radix-vue/blob/c160ae42dbb4357fc28f47db72f843348f1e3453/packages/radix-vue/src/Pagination/PaginationRoot.vue#L77

So why making API more complex if it doesn't need it? My ideal variant would be to separate knowledge about items from pagination component by removing itemsPerPage props and changing total to totalPages:

<PaginationRoot
  :total-pages="100"
  :sibling-count="1"
  show-edges
  :default-page="2"
>
	...
</PaginationRoot>

like yeah, it's just my idea on how to improve it. I will close the issue if you not interested in this

[zernonia]

@MellKam Solid reason! 👍🏻 However as you can see the pageCount is not something simple, and we don't want user to compute it themself. I would rather improve the naming of the props, and as suggested make itemsPerPage a required field.

This sounds like something we can change in v2 😉

[zernonia]

Looking into this refactor, what do you think of the changes below? Is the naming provide more clarity? 😁

[MellKam]

@zernonia I would rather just make itemsPerPage required and don't change the names to avoid breaking changes. I suggested the rename thing in the last variant because of change in logic that I proposed, since we won't be making it, I don't think it makes sense to rename then.

[zernonia]

Released in reka-ui@latest 😁. itemsPerPage prop is now required