Pagination use svelte magic

[Croug]

The Pagination component is nice, but I've noticed that in some of the frameworks, specifically Svelte, it's a little bit overcomplicated, for instance, this is how the Pagination component currently works in svelte

<script>
  const totalPages = 50
  let paging = usePagination({ offset: 1 })
  let currentPage = 1
  $: paginationPages = paging.generage(currentPage, totalPages)
  function onPageChanged(p) {
    currentPage = p
  }
  const paginationArgs = {
    onPageChange: onPageChanged,
    ...
  }
</script>

<Pagination {...paginationArgs} current={currentPage} pages={paginationPages} />

I feel this is a bit verbose and could be condensed quite a bit. First of all we can move quite a bit of usePagination into the Pagination component itself, I'm not 100% sure why it's currently a separate function, but if that's absolutely necessary, we could just accept the object with the generate function as a prop of the Pagination component. This would already clean this up a little bit.

<script>
  const totalPages = 50
  let currentPage = 1
  function onPageChanged(p) {
    currentPage = p
  }
  const paginationArgs = {
    pageGenerator: usePagination({ offset: 1 }),
    onPageChange: onPageChanged,
    ...
  }
</script>

<Pagination {...paginationArgs} current={currentPage} />

We could even go so far as to include the usePagination as the default paging generator which would also remove quite a bit.

Secondly, I think we can make use of svelte's bind keyword, to get rid of the onPageChange hook altogether. I'm not sure if you're familiar with the bind keyword, but essentially it turns a prop into a "two way" prop, meaning changes made to that prop inside of the component will replicate out. Once we implement this the code becomes significantly simpler.

<script>
  const totalPages = 50
  let currentPage = 1
  const paginationArgs = {
    pageGenerator: usePagination({ offset: 1 }),
    onPageChange: onPageChanged,
    ...
  }
</script>

<Pagination {...paginationArgs} bind:currentPage /> <!-- if the prop name and the variable name are the same, it isn't necessary to define the var name explicitly -->

If the reason it isn't like this is due to consistency across frameworks I completely understand, but if we're willing to lean into the individual frameworks a bit more, I think it would make some of the components like the Pagination component a bit more idiomatic and more natural to use for people experienced in those frameworks. As usual I am willing to create the PR if this fits into goal for the project.

[roblevintennis]

I like this idea! I think I/we could probably do similarly with Vue and v-model which does similar to svelte bind (both of which I didn't learn until recently!). Yes, I was thinking in react hooks!

Let's go for it similar--first Svelte PR then see about other frameworks as follow up.

Thanks for reaching out and offering to improve it! 🔥🙏🏽🙌🏼

[Croug]

Ok cool I'll get on this soon, what about moving usePagination into the component, is there a reason we're having users call it separately and not just doing that automatically under the hood?

[roblevintennis]

That was definitely something I'd done in Vue using the composition api. If you feel it's cleaner in Svelte to pull it into the component and still can keep it cohesive and decoupled then go for it and let's have a look at what results. I can't honestly say if that'll be better without first seeing a diff I think.

Sorry, I'm putting out a work fire atm so I have to give a briefer response then I'd like to. If you don't mind though, why don't you go for your idea to pull it in and we'll see how it looks in the PR? There also may be a chance usePagination work nice as a react hook or vue composition module but doesn't feel as idiomatic in Svelte.

[Croug]

So I think maybe we're having a communication breakdown here, because I'm not entirely sure what the reason is for having usePagination as a separate entity at all, in my mind something like this would work, but I guess I'm not 100% sure what usePagination is even doing.

<script>
  let currentPage = 1
  const paginationArgs = {
    totalPages: 50,
    offset: 1,
    onPageChange: onPageChanged,
    ...
  }
</script>

<Pagination {...paginationArgs} bind:currentPage />

Also no problem at all, I've had to put out my share of work fires haha.

[roblevintennis]

Yup, I wasn't being clear because I had forgot that in fact usePagination is not a framework-specific thing but an es6 module. In fact, since we're sharing it across frameworks it's the one thing that we've added in a separate repository here:

https://github.com/AgnosticUI/agnosticui/blob/bcdac5be57fcf81af7b20c70d1372d90bb086060/agnostic-helpers/src/usePagination.ts#L10

I think there's some tricky code that's happening there (it could certainly be cleaned up a bit) but try clicking the various paging buttons and see how they react dynamically.

I think we actually want to keep that and I think it's somewhat nice that it's shared and not duplicated in all the frameworks (DRY).

Lmk if that helps and if I can do better to clarify. Btw, I've created a gitter chat which I'll try to start getting going soon. I liked it's integration with GitHub and somehow don't love Discord or Slack.

[roblevintennis]

There may still be a way to use that agnostic-helper usePagination by refactoring the Pagination.svelte component to take it as a prop that it can call internally, and then perhaps bind:* would work. Not sure but just a high-level idea.

[Croug]

I think then I'll refactor it to do something like this

<script>
  const paginationProps = {
    pagingGenerator: usePagnination({ offset: 1 }),
    ...
  }
</script>

<Pagination {...paginationProps} />

[roblevintennis]

That could work 👍 -- perhaps we should put a comment block that pagingGenerator passed in needs to have a generate function that produces that pagination pages so following will happen within the Pagination.svelte implementation itself:

  $: paginationPages = paging.generate(currentPage, totalPages);

[roblevintennis]

So with the latest merge I think you've implemented this bugger 🏆 🥳 🔥 💪 thanks!