-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added a function to setCurrentSlideByIndex #37
base: master
Are you sure you want to change the base?
Conversation
…refs to allow other components on the page to help control the slider. Also added the relevant test
I understand if this is not on the roadmap, but figured I would be remiss to not to suggest the contribution. Against 4 other Carousel/Slider components this is the only one that properly handled all the core concerns
I would love to see this one continue to thrive. |
Hi @shaefer, thanks for using the slider and contributing! Glad to hear it covers your core requirements for such component. Some of them are among the reasons I started this project :) Can you add more details about your use case? Looks like this could be used as a starting point for implementing the "dots feature" 👍 Calling this method will not trigger animations, while using the Note that with the current implementation navigating from slide 0 to slide 3 would only animate 0 and 3, skipping completely 1 and 2, but maybe this is acceptable. I'm thinking of a way to create a fast animation which could be used for non sequential navigation, but the speed is controlled by css, so I guess this would need to be defined in there. |
@erichbehrens Thanks for the quick response! I'm happy to help out. My basic use case is just a top level nav menu that mirrors the slides (1 menu item for each slide)...so clicking on an item from the menu immediately sets that slide as the current. (Probably why I didn't mind ignoring animation initially) For all intents and purposes, it is basically the dots feature built outside the component... On that note, I agree this could easily be the beginning of the dot feature. You are also correct that I sidestepped the animation issue. The function should probably accept an optional parameter to skip animation and by default do the standard animation. I think for simplicity sake it is probably fine for the first pass to let the animation just magically shift from the current slide to the new slide and skip the intermediate steps. (This might only be a problem if you ever add a feature to display partial slides alongside the current one. Since you currently only ever display 1 slide at a time it keeps things straightforward) I'll see what it takes to get the animation working and when I have it I'll push to this branch and we'll see if it's close to what you were envisioning. |
Another way of controlling the slide index - if you don't need animations - is to rerender the component changing it's |
Added a function to
setCurrentSlideByIndex
that could be exposed via refs to allow other components on the page to help control the slider. Also added the relevant test.I wanted a way to control the component via menu buttons on the page. Since the framework to properly set the
currentSliderIndex
already existed, I was able to simply add a new functionsetCurrentSlideByIndex
(happy to use shorter name if preferred) that set the new state appropriately. Added test to show that all slides end up with the appropriate css classes. I also tested the full solution locally as well.