Fix pagination component reactivity

[ocolefed]

Description

This allows the pagination component to also dynamically change in response to the total number of pages changing.

Fixes # (issue)

236

Checklist:

• [ x ] Is this a bug fix
• [ x ] Have you submitted new feature or bugfix issue? Unless it's a small bugix, we generally prefer to track PRs against an issue which starts a dialogue to give us context.

[netlify[bot]]

✅ Deploy Preview for agnostic-astro canceled.

Name	Link
🔨 Latest commit	f8c604094330e91b33617c01b89187092f3731c6
🔍 Latest deploy log	https://app.netlify.com/sites/agnostic-astro/deploys/629d3c39c5d49d000970659e

[Croug]

Good catch! However with this new change if the total is less than the current page it could result in undefined behavior, if you're up to it could you add some guards in there to make sure that currentPage is always less than the total? Otherwise I can do it when I get home. Thanks!

[roblevintennis]

@ocolefed welcome and thanks for contributing! 💪 🚀

I think @Croug has an interesting point! 🤔 Do y'all think maybe we should do this in the useGenerator helper itself? That's here if so.

That way, all frameworks could benefit and stay simpler and the guard would be encapsulated to one place. Thoughts?

[Croug]

While I think that would be useful to have for the other frameworks, svelte would still need a bit of love because currentPage would still read an incorrect number until you interacted with pagination and we use bind to replicate out currentPage changes so at best we're displaying some incorrect information to the user and at worst it'll cause a corrupted state.

[roblevintennis]

@Croug thanks for clarification 👍 @ocolefed can you add said guard on this PR?

[ocolefed]

Hi all

I've updated the PR with a guard added to prevent the current page from exceeding the total. Originally, I had this guard inside the parent component which contained the Pagination, but it does make more sense to have it inside the Pagination component itself.

I had a look at implementing this in the pagination generator helper, but as @Croug mentioned this doesn't work (at least with svelte) since it can't actually change the value of current within the pagination component. It would need to be modified to return a (potentially) changed current as well as the string[] which is rendered into buttons.

[roblevintennis]

Thanks @ocolefed 🙏🏽

I'm wondering if we should reassign current to total (err on side of fault tolerance but possibly mask bugs in the consumer code)? Or, do that plus warn. Or, just throw an error with a descriptive message so they can correct their code. Cc @Croug too.

Thanks I'd like to hear y'all thoughts

[Croug]

I think it's a good idea to set current to total only when current>total. And a warning should be thrown in the console on this occurrence

[roblevintennis]

@ocolefed sorry for back and forth -- do you mind just adding a console.warn per the discussion above. I think we'll be good to merge after that...thanks in advance!

[ocolefed]

No problem @roblevintennis , back and forth is how OSS works :stuck_out_tongue:

I've added the warning message now, I'm not 100% on the text but I wasn't able to find many other similar examples in the codebase.

[roblevintennis]

@ocolefed hrm, you're PR just got a lot bigger with CSS and linting changes it looks like. Even if those are legit I would like to do that sort of updates separately if possible. Can you back out HEAD~1 and redo it with just the console.warn statement please? The messaging was fine fwiw. Thanks!

[ocolefed]

Ahh! Whoops, the classic difference between "save" and "save without formatting" has caught me again. I'll revert those changes and include just the actual changes I meant to introduce... and now it looks a lot neater

[roblevintennis]

Thanks @ocolefed 💯 🚀