Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shaefer
Copy link

@shaefer shaefer commented Apr 30, 2018

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 function setCurrentSlideByIndex (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.

…refs to allow other components on the page to help control the slider. Also added the relevant test
@shaefer
Copy link
Author

shaefer commented Apr 30, 2018

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

  • CSS works when embedded inside various layout schemes
  • responsive styling is straightforward to manage via media queries
  • swipe support on mobile
  • the component itself is crafted using modern design, standards, and testing

I would love to see this one continue to thrive.

@erichbehrens
Copy link
Owner

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 goTo method would do so. You need to pass the animation direction to goTo, but this could be (easily) computed. Maybe you could change setCurrentSlide to compute the animation direction and call goTo. Does it make sense for you use case?

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.

@shaefer
Copy link
Author

shaefer commented May 1, 2018

@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.

@erichbehrens
Copy link
Owner

Another way of controlling the slide index - if you don't need animations - is to rerender the component changing it's slideIndex prop from the parent or context. This would actually be the react way of doing things.

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

Successfully merging this pull request may close these issues.

None yet

2 participants