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

Children not being removed #38

Closed
DiogoDoreto opened this issue Apr 29, 2016 · 10 comments
Closed

Children not being removed #38

DiogoDoreto opened this issue Apr 29, 2016 · 10 comments
Labels

Comments

@DiogoDoreto
Copy link

DiogoDoreto commented Apr 29, 2016

I've found 3 cases where children are not being removed from DOM:

  1. When disableAllAnimations is true.
  2. When using a browser that do not support CSS transitions.
  3. Firefox ( jsbin )

Edit: added Firefox case.

@joshwcomeau
Copy link
Owner

joshwcomeau commented May 1, 2016

Hi - Thanks for the report!

Will definitely push out a fix shortly for the first concern; that's an awful bug!

I'm not too concerned about supporting browsers lacking CSS transitions; it seems like all modern browsers do, and I'm pretty sure other things would be broken in IE 9 and lower :)

@DiogoDoreto
Copy link
Author

I agree about not fully supporting browsers that don't have CSS transitions, but since React itself do support IE 9, I think that it's a good idea to a least not break the usability.

I have the requirement of supporting IE 9 on my project. Right now I'm testing if CSS transitions exists and if not, I set both enter/leave transitions to none (will change to set the value of disableAllAnimations when the bug is fixed).

So, my suggestion is to automatically disable animations if browser doesn't support CSS transitions.

@joshwcomeau
Copy link
Owner

joshwcomeau commented May 4, 2016

Hey @DiogoDoreto!

So, I took your suggestion and disabled all animations by default when CSS transition support isn't detected.

Here's a PR: #43

Small detail: I'm using onTransitionEnd as the mechanism for telling if CSS transitions are available or not, which may be subtly different than support for CSS transitions in general. I did some quick research and it looks like the support is similar enough, though.

I also published it as a pre-release, version 2.1.3-beta1

Can you please test this out and see if it works in your project? It works for me in the storybook, but I haven't tested it outside of that :)

EDIT: also, aside from this, does Flip Move work in IE9? I have never even tested that, I just assumed it wouldn't work. Awesome if it does!

@DiogoDoreto
Copy link
Author

Hey! I'm sorry, I'll be able to test this only tomorrow. But looking at the code, I think that this check should be enough (and even preferable since that is what your code really needs).

But it's worth noting that the test is only relevant when the first item of this issue is fixed.

Aside from this, yes FlipMove is "working" in IE9 if I set animations to 'none'... I'm using the virtual machine from modern.ie to test :)

@DiogoDoreto
Copy link
Author

@joshwcomeau I found out that Firefox (v45.0.2, Win7/10) is not removing elements as well. I made a small example: http://jsbin.com/cuceciyega/edit?js,console,output

If you open in Chrome, you will see different number of dom elements being logged. If you open on Firefox, it's always 10.

@DiogoDoreto
Copy link
Author

DiogoDoreto commented May 5, 2016

Researching a bit more about Firefox, the problem seems to be in this line. Firefox doesn't include the srcElement property to the transitionend event.

Edit: according to MDN[1], this is just an alias to event.target, so it's better to use the standard property.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Event/srcElement

@joshwcomeau
Copy link
Owner

joshwcomeau commented May 5, 2016

Damn @_@ no clue where I got srcElement from.

Thanks for pointing it out! BTW if it interests you, you can definitely submit PRs for this! You found the bug, you should get credit for it. Otherwise I'm happy to do it myself.

For the original issue: I believe this change fixes both 1. and 2.! The difference is that before, when no animations were necessary, I was just aborting prematurely. Now, I'm aborting prematurely, but also setting the state to be equal to the props. This means that FlipMove will simply defer to the children passed in through props, and render them as-is.

Let me know when you've had opportunity to test, and if you'd like me to implement the srcElement fix

@DiogoDoreto
Copy link
Author

Hey, just tested here and it's working! :D

I don't mind, you can change this yourself...

Thank you very much!
Looking forward for the new version release now ;)

@joshwcomeau
Copy link
Owner

Ok! Finally had some time this morning, and I pushed fixes to the three issues found :) published v2.1.4

Thanks again for all your help @DiogoDoreto! Please let me know if you encounter any more weirdness!

@DiogoDoreto
Copy link
Author

Thank you for quick responses and fixes! 👍

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

No branches or pull requests

2 participants