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

Fix transition scale bug #110

Merged
merged 1 commit into from
Mar 19, 2019
Merged

Conversation

besLisbeth
Copy link
Collaborator

Resolve #108

@schiehll
Copy link
Owner

Thank you as always @besLisbeth!

@schiehll schiehll merged commit 065b8eb into schiehll:master Mar 19, 2019
@schiehll
Copy link
Owner

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

@besLisbeth
Copy link
Collaborator Author

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

@schiehll
Copy link
Owner

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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Owner

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

@besLisbeth
Copy link
Collaborator Author

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
Copy link
Collaborator Author

I can't understand what caused such strange behaviour

@schiehll
Copy link
Owner

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
Copy link
Owner

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

@besLisbeth
Copy link
Collaborator Author

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

@besLisbeth
Copy link
Collaborator Author

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

@schiehll
Copy link
Owner

Weird stuff hahah can I see some code?

@besLisbeth
Copy link
Collaborator Author

besLisbeth commented Mar 19, 2019

Try to pull from there and not include react-select

@besLisbeth
Copy link
Collaborator Author

Code can be dirty)

@schiehll
Copy link
Owner

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

@besLisbeth
Copy link
Collaborator Author

Thank you!

@schiehll
Copy link
Owner

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
Copy link
Collaborator Author

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

I'll try to find a solution.

@schiehll
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants