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

DOM Element Not Removed After Animation #215

Closed
mathewbyrne opened this issue Aug 30, 2018 · 4 comments
Closed

DOM Element Not Removed After Animation #215

mathewbyrne opened this issue Aug 30, 2018 · 4 comments

Comments

@mathewbyrne
Copy link

When animating out an unmounted element with Transition, the DOM elements are not removed after the animation.

Now unfortunately I cannot isolate and reproduce this issue. Here is a sandbox with an attempt to isolate, however the DOM elements are removed after the animation — https://codesandbox.io/s/oooxx0njl6

Here is the code snippet from the application:

<Transition
    native
    from={{ opacity: 0 }}
    enter={{ opacity: 1 }}
    leave={{ opacity: 0 }}
>
    {this.state.display &&
        (style => (
            <animated.div style={style}>
                <a onClick={() => this.setState({ displayBriefs: false })}>Back</a>
            </animated.div>
        ))}
</Transition>

And lastly here is an animation showing the issue from our application:

There look to be a few related issues, I've definitely seen #211 and #134 occurring with this setup. Seems similar to #43 which was resolved in a previous version.

We're using react-spring: ^5.6.9 and react: ^16.4.2.

@drcmda
Copy link
Member

drcmda commented Aug 30, 2018

Are you 100% sure it uses react and especially react-dom 16.4.x (because that one has been the culprit in the past many times)?

If it's gDSFP indeed, i have asked the react team to please unpublish 16.3.x. Dan Abramov said the issue is too minor for that, since going to loose on unlublishing would take away from the seriousness of it. See: facebook/react#12898 (comment)

@mathewbyrne
Copy link
Author

Ah thankyou, that was it we didn't have react-dom up to 16.4 alongside react. I missed #198 which looks like the same issue. Thanks again.

@gaearon
Copy link

gaearon commented Aug 30, 2018

I mean, we can’t unpublish 16.3 because it would completely break projects. We also can’t do that technically — I’m sure npm wouldn’t allow us to unpublish something that’s actively used.

We could deprecate 16.3. But it will be a pain for thousands of people who migrate minor by minor. They might decide to go back to 16.2 instead of pushing forward. That’s even worse.

So we’d rather have them upgrade to 16.3, and if they have issues, upgrade further. This period will pass.

You can test for React.version and display a message for all 16.3 releases. This won’t help with detecting ReactDOM version but can be close enough.

You can also set ^16.4.0 as your peer dependency to make Yarn/npm warm when earlier versions are used.

@drcmda
Copy link
Member

drcmda commented Aug 30, 2018

@gaearon Hi Dan! Peer-dependencies should be set, i hope it warns users. I understand it's not really an option to un-publish, like you said, it will pass. I'll add a warning message in transition using React.version, this is a really good idea.

10xjs added a commit to sensu/sensu-go that referenced this issue Sep 4, 2018
react-spring is directly inspired by the API of react-motion but has much better performance

Upgrading to latest version of react due to a bug in 16.3.x
  see:
    pmndrs/react-spring#136 (comment)
    pmndrs/react-spring#215 (comment)
    https://reactjs.org/blog/2018/05/23/react-v-16-4.html#bugfix-for-getderivedstatefromprops

Signed-off-by: Neal Granger <neal@nealg.com>
10xjs added a commit to sensu/sensu-go that referenced this issue Sep 6, 2018
react-spring is directly inspired by the API of react-motion but has much better performance

Upgrading to latest version of react due to a bug in 16.3.x
  see:
    pmndrs/react-spring#136 (comment)
    pmndrs/react-spring#215 (comment)
    https://reactjs.org/blog/2018/05/23/react-v-16-4.html#bugfix-for-getderivedstatefromprops

Signed-off-by: Neal Granger <neal@nealg.com>
10xjs added a commit to sensu/sensu-go that referenced this issue Sep 18, 2018
## What is this change?

### Replace react-motion with react-spring

React-spring is directly inspired by the API of react-motion but has much better performance

Upgrading to latest version of react due to a bug in 16.3.x
  see:
   - pmndrs/react-spring#136
   - pmndrs/react-spring#215
   - https://reactjs.org/blog/2018/05/23/react-v-16-4.html#bugfix-for-getderivedstatefromprops

### Create "relocation" system

This PR introduces "relocation" as more flexible alternative to `React.Portal` supporting both declarative (`Relocation.Sink`) and imperative (`Relocation.Consumer`) interfaces.

The intention is to eventually rely on the published `relocation` package once it is updated to match the pattern used here.

Showing a toast notification:

```jsx
const SomeComponent = props => (
  <ToastProvider>
    {({ addToast }) => (
      <button
        onClick={someAsyncAction.then(
          result =>
            addToast(({ id, remove }) => (
              <Toast
                variant="success"
                message={`Action succeeded with result: ${result}`}
                onClose={remove}
                maxAge={5000} // 5 seconds
              />
            )),
          error =>
            addToast(({ id, remove }) => (
              <Toast
                variant="error"
                message={`Action failed with error: ${error}`}
                onClose={remove}
                maxAge={5000} // 5 seconds
              />
            )),
        )}
      >
        Fire Async Action
      </button>
    )}
  </ToastProvider>
);
```

### Add feedback toast for check execution 

Displays a toast indicating pending and success states for each requested check execution.

![check toast](https://user-images.githubusercontent.com/1074748/45127989-58227580-b130-11e8-9995-1aba1f1e071b.gif)

## Why is this change necessary?

Closes #1932 
Closes #2012

## Does your change need a Changelog entry?

No

## Do you need clarification on anything?

No

## Were there any complications while making this change?

No

## Have you reviewed and updated the documentation for this change? Is new documentation required?

No
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

No branches or pull requests

3 participants