Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Feature Request: Add prevDisabled and nextDisabled #28

Closed
jasongerbes opened this issue Sep 9, 2024 · 4 comments · Fixed by #29
Closed

Feature Request: Add prevDisabled and nextDisabled #28

jasongerbes opened this issue Sep 9, 2024 · 4 comments · Fixed by #29

Comments

@jasongerbes
Copy link
Contributor

Rendering previous and next buttons current requires using the activePageIndex and pages.length to determine whether each button should be disabled. For instance:

const { prev, next, activePageIndex, pages } = useSnapCarousel();

const prevDisabled = activePageIndex <= 0;
const nextDisabled = activePageIndex === pages.length - 1;

As a nice-to-have API improvement, useSnapCarousel could use activePageIndex and pages.length internally to include prevDisabled and nextDisabled properties in the SnapCarouselResult:

const { prev, next, prevDisabled, nextDisabled } = useSnapCarousel();

I'd be happy to create a PR for this feature

@richardscarrott
Copy link
Owner

Hi @jasongerbes -- yeah I think that makes sense as it is a little annoying having to read that logic so often.

I think returning hasPreviousPage and hasNextPage might read better as I can imagine use cases outside of disabling the button -- I have this in a carousel of mine:

const hasPreviousPage = activePageIndex > 0;
const hasNextPage = activePageIndex < pages.length - 1;

Happy to accept a PR for it 👍

@jasongerbes
Copy link
Contributor Author

Hey @richardscarrott, thanks for that. I'll get a PR up shortly.

Regarding naming, would you prefer:

  1. hasPreviousPage and hasNextPage (as suggested)
  2. hasPrevPage and hasNextPage
  3. hasPrev and hasNext

I feel 2 or 3 would be more consistent with the naming of the prev/next functions

@richardscarrott
Copy link
Owner

Yeah, let’s go with 2 🙏

@jasongerbes
Copy link
Contributor Author

@richardscarrott, PR #29 is ready for your review

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants