Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

useSignalEffect is not working as expected when using visibility: hidden/visible in CSS animations #237

Closed
SantosGuillamot opened this issue May 23, 2023 · 4 comments
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@SantosGuillamot
Copy link
Contributor

The custom useSignalEffect implementation is needed to wait for DOM changes before calling the passed callback. And, while it works great in most of the use cases, we discovered that it doesn't work as expected when visibility: hidden/visible is included in CSS animations.

This issue was discovered in the lightbox pull request. And this is a video with a longer explanation of the problem:

https://www.loom.com/share/63bb6d517f2a47398101d15be6b70db6

I assume we will have to triage why this is happening and find out an implementation where this is supported.

@SantosGuillamot SantosGuillamot added the bug Something isn't working label May 23, 2023
@luisherranz
Copy link
Member

I wonder if it works with (P)React's useEffect.

@SantosGuillamot
Copy link
Contributor Author

I tried to check if it works with React's useEffect and it seems the behavior is the same and it also has this issue. Not sure if I am testing properly, though.

I created this StackBlitz with the code I used and I recorded this video explaining what I tested:

https://www.loom.com/share/5b972b0fe63f41e3b94f4d0fce3525b0

@luisherranz
Copy link
Member

Thanks for checking it out, Mario 🙂 I tested your example in Preact, and it works exactly like that.

So, as this doesn't seem like a problem in useSignalEffect because it should behave like useEffect, can we close this as wont-fix? Is the workaround you showed in the video a viable one?

@SantosGuillamot
Copy link
Contributor Author

can we close this as wont-fix?

Yes, I believe we can close it then. If it hasn't been an issue in (P)React so far, maybe we shouldn't worry about it. At least at this moment.

Is the workaround you showed in the video a viable one?

For this simple use case, and for the lightbox, it is totally viable. The visibility property is not needed. I don't know about more complex use cases though.

@SantosGuillamot SantosGuillamot added the wontfix This will not be worked on label May 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants