electerious / basicLightbox

The lightest lightbox ever made.
https://basiclightbox.electerious.com
MIT License
557 stars 55 forks source link

Listen for transition end #12

Open jimblue opened 6 years ago

jimblue commented 6 years ago

You're using requestAnimationFrame two times but without changing dom propriety from javascript. I don't see how this can improve performance as you add class to animate dom element.

I can be wrong of course, if it's the case please light me candle 👍

https://github.com/electerious/basicLightbox/blob/e82ed1060f1a48e068d4a296eb29d741214a9b37/src/scripts/main.js#L123 https://github.com/electerious/basicLightbox/blob/e82ed1060f1a48e068d4a296eb29d741214a9b37/src/scripts/main.js#L150

electerious commented 6 years ago

The first requestAnimationFrame is there to ensure that the class change triggers the animation. You have to wait a frame after adding the element to the DOM. Otherwise it wouldn't do the transition to the basicLightbox--visible class.

I'm not sure if there's a reason for the second requestAnimationFrame. I would say that it's useless. Might be worth a try. We could also use a keyframe animation and avoid requestAnimationFrame completely.

jimblue commented 6 years ago

Ok I got it, thanks you 😄 !

Also I think you could provide a better callback here: https://github.com/electerious/basicLightbox/blob/ab2a2df6378c50946dc45b315d9d4268f4e741a7/src/scripts/main.js#L154

Why don't you listen for animation end?

electerious commented 6 years ago

Removed the second requestAnimationFrame: https://github.com/electerious/basicLightbox/blob/master/CHANGELOG.md#401---2018-02-23

Listening for animation end would be better. PR welcome :)