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

No animation on modal mounting #607

Closed
Rootmafia opened this issue Oct 2, 2017 · 17 comments
Closed

No animation on modal mounting #607

Rootmafia opened this issue Oct 2, 2017 · 17 comments

Comments

@Rootmafia
Copy link

Rootmafia commented Oct 2, 2017

## Issue description

  • version "reactstrap": "next",
  • components: Modal
  • react version "^16.0.0",

There is no fade in animation on Modal appearing, have you removed it?

@virgofx
Copy link
Collaborator

virgofx commented Oct 2, 2017

This is a valid bug we need to investigate further.

@devonjs
Copy link

devonjs commented Oct 3, 2017

@virgofx hello! could i take a stab at this one? been wanting to contribute for a while, would investigate tomorrow after work!

@TheSharpieOne
Copy link
Member

@devonjs go for it. There is a branch which updates the dev version of react in this project to 16. You'll probably need to branch off of that or something to see the issue since this only happens with react 16.

@virgofx
Copy link
Collaborator

virgofx commented Oct 3, 2017

I believe the issue is we need to force a repaint on the initial load once it applies the first class. It looks like CSSTransition has this built in; however, we opted to use Transition, so it may behoove us to switchover; however, we need to update this everywhere and ensure we use the bootstrap version of the classes (e.g. show). (If of course, this is the issue and it fixes it -- otherwise need more investigation)

@TheSharpieOne
Copy link
Member

There is an issue which was recently open with react-transition-group: reactjs/react-transition-group#216. Not sure if it is valid or not, but it seems similar to what we are experiencing and thus we may be able to benefit from the solution to it; regardless of whether it is an issue with react-transition-group or not

@devonjs
Copy link

devonjs commented Oct 3, 2017

@virgofx @TheSharpieOne Great, thanks for the direction on this issue. Will consider what you both mentioned above when approaching this!

@devonjs
Copy link

devonjs commented Oct 5, 2017

hmmm there seems to be a couple of things to consider for React 16:

  1. As mentioned, react-transition-group seems to not work as intended
  2. Seems ReactDOM.createPortal would be the preferred way to accomplish what ReactDOM.unstable_renderSubtreeIntoContainer does react/issues/10309, documentation of what it does seems this way as well

ReactDOM.createPortal is such a big feature in React 16 that for this to play with React 16 and its future versions it should be used, what do you think?

It also seems like CSSTransition got bundled into react-transition-group in the react documentation, not too sure if switching to CSSTransition would achieve better results, I'd have to do a little more research into it.

@TheSharpieOne
Copy link
Member

TheSharpieOne commented Oct 5, 2017

#601 has the task of upgrading to portals, but to better maintain compatibility with react <16 the suggestion is to use createPortal and fallback to unstable_renderSubtreeIntoContainer if it is not there. material did something similar
As for Transition vs CSSTransition Yes, we are using react-transition group, The doc you reference are old, it itself references react-transition-group v1. The v2 docs can be found here. I think the preference is for everything to use Transition to be consistent.

devonjs pushed a commit to devonjs/reactstrap that referenced this issue Oct 11, 2017
devonjs pushed a commit to devonjs/reactstrap that referenced this issue Oct 11, 2017
@devonjs
Copy link

devonjs commented Oct 11, 2017

@TheSharpieOne sent PR, I'll leave the createPortal and unstable_renderSubtreeIntoContainer fallback to who ever picks up #601

@jonathanazulay
Copy link

I was gonna file this issue and reproduced it here. Also reproduces #360

https://codesandbox.io/embed/8x3nm1l659

@chrisparton1991
Copy link

To clarify, by having no animation on modal mounting are you saying that the modal instantly appears, or that it remains invisible?

I'm experiencing an issue where my modal has a fade class attached that never gets removed, leaving it invisible. If that's the issue here, I won't raise a dupe.

@TheSharpieOne
Copy link
Member

The fade class stays the entire time the modal exists. This is how bootstrap does it. An additional class is added to affect the opacity.

@Kielan
Copy link

Kielan commented Jan 1, 2018

@TheSharpieOne Any update on this? I see the commit referencing this issue 2 months ago is just one line long. I have set up modals a few times in react and want to give it a go but I see two separate pr's, one which is not merged.

@jonathanazulay
Copy link

@Kielan Seems to be fixed in v5.0.0-alpha4, it appears to be a fade in and out on my iPhone

@virgofx
Copy link
Collaborator

virgofx commented Jan 1, 2018

I also believe this is fixed. There may be a few weird cases with large timeouts, but all of my use cases seem fixed.

@jonathanballs
Copy link

I can confirm that is fixed for me.

@TheSharpieOne
Copy link
Member

Fixed in v5, closing.

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

No branches or pull requests

8 participants