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

Check className has a truthy value before trying anything else #108

Merged
merged 4 commits into from
Sep 30, 2020
Merged

Check className has a truthy value before trying anything else #108

merged 4 commits into from
Sep 30, 2020

Conversation

msallent
Copy link
Contributor

We recently had an issue on one of our projects where IE11 would crash and go entirely blank. After some hours of debugging, we found the culprit, which was setting selectedClass as an empty string.

This PR intends to fix that by first checking if className has a truthy value before moving forward.

To give a little bit more of context on the issue and how it happened:

We have a useSlider hook that wraps useEmblaCarousel and adds some extra functionality, and seeing as we were not using the selectedClass, we initialized Embla's hook like this:

const [setEmblaSliderRef, emblaAPI] = useEmblaCarousel({
  selectedClass: "",
  ...options,
});

Unfortunately the project's repo is private and we can't share any of our staging environments for you to test this. I tried forking the CodeSandbox specified in the CONTRIBUTIONS document but it threw out a 404. Anyways, this shouldn't be too hard to replicate for you to test.

If you need any additional information, please let me know. Thank you!

@davidjerleke
Copy link
Owner

davidjerleke commented Sep 29, 2020

Hi Matias (@msallent),

Thank you for taking time to create this PR. So if I understand you correctly:

  • The page crashed on IE11 because of the reason you mention.
  • You opted out of selectedClass because you don’t need to use it in your project?

Is this a show stopper for your project?

Thanks again for this PR. I will merge it and release it as soon as possible. Also, thanks a lot for pointing out the broken link in the contribution document. I will update it soon.

Best,
David

@msallent
Copy link
Contributor Author

Hey @davidcetinkaya!

Not a show stopper, thankfully. We just decided to remove setting selectedClass as an empty string and move on, but I first wanted to take five minutes and open a PR as we've been using your package with no issues at all.

Thank you for the awesome carousel!

@davidjerleke davidjerleke merged commit 4140eb8 into davidjerleke:master Sep 30, 2020
@davidjerleke
Copy link
Owner

davidjerleke commented Sep 30, 2020

Thanks again Matias (@msallent).

I've merged your PR now so welcome to the contributors club 🙂. Release v4.0.5 comes with your fix. Please update the embla-carousel package to the latest version.

Have a great day!
David

@davidjerleke davidjerleke added bug Something isn't working resolved This issue is resolved labels Sep 30, 2020
@msallent msallent deleted the bug/ie11-empty-string-class branch September 30, 2020 13:55
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

Successfully merging this pull request may close these issues.

2 participants