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

Unnecessary scroll under certain conditions #46

Closed
olushchik opened this issue Jan 29, 2020 · 11 comments
Closed

Unnecessary scroll under certain conditions #46

olushchik opened this issue Jan 29, 2020 · 11 comments
Labels
bug Something isn't working resolved This issue is resolved

Comments

@olushchik
Copy link

Hi! I ran into a nasty bug with unnecessary scroll under certain conditions:

  • loop: true
  • draggable: false
  • slidesToScroll: more than 1 (for example 4)
  • slides count: more than slidesToScroll but not completely divisible by slidesToScroll (for example 5)

In this case after initial load when we scroll to next and try to click link on the first slide we see unnecessary scroll.

Here's a demonstration of what I'm talking about: https://codesandbox.io/s/embla-carousel-containscroll-mec6x

As I figured out it is because click on the link fires focus event and Embla considers that this is not the current slide and does scroll.

@davidjerleke davidjerleke added the bug Something isn't working label Jan 29, 2020
@davidjerleke
Copy link
Owner

davidjerleke commented Jan 29, 2020

Hi Oleg (@olushchik),

Nice catch, and thank you for reporting this! I appreciate the clear CodeSandbox demonstration ⭐️.

As you mention, Embla acts on the focus event and wants to scroll to the snap index containing the focused element (in this case index 0). This behaviour is desired when a user navigates with the tab key for accessibility reasons. However, it's not desired when clicking with a pointer.

Right now I'm working on the scrollBy() feature and it might take some time for me to investigate this further in order to release a patch. In the meantime, feel free to submit a pull request if you want.

Best,
David

@olushchik
Copy link
Author

Hi, David! Thanks for the response!

This behaviour is desired when a user navigates with the tab key for accessibility reasons.

Oh, now I understand for what reasons this was done 😃 But I'm not sure that I understand how to fix it properly 😢

@davidjerleke
Copy link
Owner

Hello Oleg (@olushchik),

No worries 🙂. This issue will have to wait then. I appreciate you taking time to report this!

Kindly,
David

@davidjerleke davidjerleke added the investigating Issue is being looked into label May 16, 2020
@davidjerleke davidjerleke removed the investigating Issue is being looked into label Jun 6, 2020
davidjerleke pushed a commit that referenced this issue Jun 6, 2020
@davidjerleke
Copy link
Owner

Hello Oleg (@olushchik),

The following commit includes a fix for this bug. Embla Carousel Version 3 is just around the corner so the bug fix will be released with version 3.

Thanks for taking time to create this issue 👍!

Best,
David

This bug has been fixed and will be released with version 3.

@davidjerleke davidjerleke added resolved This issue is resolved next major version labels Jun 6, 2020
@olushchik
Copy link
Author

Thanks a lot, you are the best!

@davidjerleke
Copy link
Owner

Hello @olushchik,

This bug has been fixed and is ready for test if you grab the latest version. Note that it's a major version release so please read the instructions here.

I would very much appreciate if you could confirm if this is working as expected now and the bug has been solved.

@davidjerleke davidjerleke added awaiting response Issue is awaiting feedback and removed next major version labels Jun 12, 2020
@davidjerleke
Copy link
Owner

Have you had the chance to test this @olushchik? Thanks in advance.

@olushchik
Copy link
Author

Unfortunately, no. Just tested in sandbox, works fine.

@davidjerleke
Copy link
Owner

@olushchik so this issue is resolved then 🙂?

@olushchik
Copy link
Author

I reread my message and realized that it was not clear)) I meant that I did not check this in my real project, but only in the sandbox. And looks like the issue is resolved 👍

@davidjerleke
Copy link
Owner

Awesome, thank you for your feedback. I'm closing this issue and thanks for reporting this 👍.

@davidjerleke davidjerleke removed the awaiting response Issue is awaiting feedback label Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working resolved This issue is resolved
Projects
None yet
Development

No branches or pull requests

2 participants