search

schiehll / react-alert

alerts for React
MIT License
607 stars 99 forks source link

Fix transition scale bug #110

Closed besLisbeth closed 5 years ago

besLisbeth commented 5 years ago

Resolve #108

schiehll commented 5 years ago

Thank you as always @besLisbeth!

schiehll commented 5 years ago

Oh, actually, after testing it, looks like it broke the scale transition, will revert it.

besLisbeth commented 5 years ago

Oh no, I also tested it and did not find anything. Sorry then(

schiehll commented 5 years ago

No worries! I've tested it in the codesandbox and it broke the scale transition when the alert is leaving. Reverting it fixed. BUT looks like it fix only when there's more than one alert, when there's only one it's still broken. 🤔

We should investigate it.

schiehll commented 5 years ago

I've just tested the v5.1.0 (before the addition of support for multiple positions) and it worked...looks like it's broken since v5.2.0. Can you take a look in the code you added in that release and see what could be causing it?

besLisbeth commented 5 years ago

Yes, I found it. The problem with animation for the ONE leaving alert caused by condition rendering - react unmount TransitionGroup faster than it shows the exiting animation

schiehll commented 5 years ago

Yeah, that make sense! Did you thought in any possible solution?

besLisbeth commented 5 years ago

Yes! I move TransitionGroup higher on the DOM tree - to be stable and above the conditionally rendered Wrappers. But still, have problems with exiting animation - I'm trying to find the solution with 'react-select'

besLisbeth commented 5 years ago

I can't understand what caused such strange behaviour

schiehll commented 5 years ago

I'm not so worried about the react-select thing, for all we know it can be a problem with them, I'm more worried with the broken transition thing.

schiehll commented 5 years ago

Moving it higher in the tree makes it work not considering the react-select issue?

besLisbeth commented 5 years ago

It's strange. Now it works for the last alert in the array. But not for others.

besLisbeth commented 5 years ago

The alerts between 1 and alerts.length are not firing onExit event

schiehll commented 5 years ago

Weird stuff hahah can I see some code?

besLisbeth commented 5 years ago

Try to pull from there and not include react-select

besLisbeth commented 5 years ago

Code can be dirty)

schiehll commented 5 years ago

that's totally fine! will take a look and see if I can help somehow, thanks!

besLisbeth commented 5 years ago

Thank you!

schiehll commented 5 years ago

Hey @besLisbeth! I took a look in the code and couldn't find a solution. So for now I will revert the changes of v5.2, so you would only be able to use multiple alerts via multiple Contexts (unless you stay in v5.2.0 - v5.3.1).

If you find a solution, we can put the feature back.

besLisbeth commented 5 years ago

Sure, thank you. Sorry that I caused such inconvenience(

I'll try to find a solution.

schiehll commented 5 years ago

No worries! If you find a solution you are more then welcome to submit a new PR!