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

Carousel - Running SlideTo on active item results in hiding data #227

Closed
Vuurvlieg opened this issue Jul 10, 2018 · 10 comments
Closed

Carousel - Running SlideTo on active item results in hiding data #227

Vuurvlieg opened this issue Jul 10, 2018 · 10 comments
Labels
enhancement potential improvement

Comments

@Vuurvlieg
Copy link

Vuurvlieg commented Jul 10, 2018

Hi, its me again.

This issue is similiar like the Tabs in #187, but this time its affecting the Carousel component. When sliding to a already active item, the active item becomes hidden, which is wrong.

To reproduce:

  1. Create a HTML Carousel markup without data attributes
  2. Use JavaScript to initialize a Carousel instance. Disable automatically sliding using interval: false
  3. Use the build-in function slideTo and gives its current active item index.
  4. The result is that the currently active item will be hidden, which shouldn't

Check this example. Click the button to open the modal and then click the reset button. The result is that the active carousel item will be hidden.

At least this is for V4 (not tested for V3).

@thednp
Copy link
Owner

thednp commented Jul 10, 2018

I don't think I understand the issue.

@Vuurvlieg
Copy link
Author

The issue is that the active carousel disappears, just like the mentioned tab issue..
When sliding to a already active caoursel item, this item should not be hidden.

In my codepen example, the default carousel item index is 0. When you click the reset button, the carousel item with index 0 should stay visible instead of hidden.

@thednp
Copy link
Owner

thednp commented Jul 11, 2018

OK I begin to understand. If you go next slide it always goes nicely but if you do slideTo(0) when you're already at the slide 0 it will hide. TRUE. It's broken and will not be able to handle.

However, this is a case where other carousel scripts try and duplicate slides for various reasons like swipe or simply making it able to work with anything you throw at it.

We won't do that. We understand the context and manage these cases in most efficient manner to avoid breaking the basic functionality. For instance in this particular case here's what to do: we need to filter that specific case like so:

function resetSlides() {
   if ( myCarouselInit.getActiveIndex() !==0 ) { // this is now required
     myCarouselInit.slideTo(0);
   }
}

So like many others, this is a case of user related issue, nothing to do with our script, so I'm going to close this, hoping the above will help you understand how to manage these cases.

@thednp thednp closed this as completed Jul 11, 2018
@thednp thednp added the invalid not related or critical label Jul 11, 2018
@Vuurvlieg
Copy link
Author

@thednp
I understand your solution which does work fine for this case. However, the project Im working on, this solution will fail, simply because sometimes I have to reset the Carousel to it's default active index, which doesn't have to be 0 . I mean, sometimes the reset button should slide to slide index 2 for example, just because index 2 was the default active slider at page load.

I know this is very specific, but a really easy solution could be added to this library to fix this kind of issue. Just check if the current item index is equal to the index of the next slide, if it is just return from function.

All you have to do is go to the slideTo function and add the following:
if (activeItem == next) { return; }

This will fix this issue completely. Please check line 595 of the JS in this fixed example

@thednp
Copy link
Owner

thednp commented Jul 11, 2018

Perfectly acceptable solution. That's the spirit!

Thanks

@Vuurvlieg
Copy link
Author

Thanks, but are you going to include this fix into the library, would be awesome!?

@thednp
Copy link
Owner

thednp commented Jul 11, 2018

Yes @Vuurvlieg

@thednp thednp added enhancement potential improvement and removed invalid not related or critical labels Jul 11, 2018
@Vuurvlieg
Copy link
Author

Awesome, thank you so much!

thednp added a commit that referenced this issue Jul 12, 2018
* fixing #227, V3/V4 Carousel cannot use `slideTo` to jump to the current active item
* documentation updates regarding #227
* version bump
thednp added a commit that referenced this issue Jul 12, 2018
* fixing #227, V3/V4 Carousel cannot use `slideTo` to jump to the current active item
* documentation updates regarding #227
* version bump
@thednp
Copy link
Owner

thednp commented Jul 12, 2018

@Vuurvlieg please test latest master and check the changes

@Vuurvlieg
Copy link
Author

@thednp I've tested the latest master and everything works fine, thank you so much!

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

No branches or pull requests

2 participants