Skip to content

Conversation

karlwills
Copy link

@karlwills karlwills commented Feb 25, 2020

This pull request relates to #341 and ensures that only the *-enter-done and *-exit-done classes should exist on the tested DOM node when config.disabled is set totrue

Credit goes to @joefurniss for spotting this issue.

Testing Components with Transitions

https://reactcommunity.org/react-transition-group/testing/

@karlwills karlwills requested a review from jquense February 25, 2020 14:47
@karlwills karlwills changed the title Remove enter and exit classes if config.disabled is true Ensure only the *-enter-done and *-exit-done classes should exist tested DOM node when config.disabled is set totrue Feb 25, 2020
@karlwills karlwills changed the title Ensure only the *-enter-done and *-exit-done classes should exist tested DOM node when config.disabled is set totrue Ensure only the *-enter-done and *-exit-done classes should exist tested DOM node when config.disabled is set to true Feb 25, 2020
@taion
Copy link
Member

taion commented Feb 27, 2020

This doesn't actually fix #341, though, does it? That one doesn't just cover cases with config.disabled?

@karlwills
Copy link
Author

@taion No it doesn't fix that issue, It does relate to it though.

However, it does fix the issue I was having (and feel that should be fixed in the codebase) with both *-enter-done and *-exit-done classes appearing when testing components that rely on CSSTransition

Copy link
Member

@taion taion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This seems somewhat insufficient, though. Among other things we're not removing appear. And it'd be straightforward to deal with the case when exit is set to false.

Given that, I think it'd make the most sense to just unconditionally remove those classes...

Alternatively, maybe <Transition> can always call onExit.

@jquense thoughts?

this.addClass(node, type, 'done');

if (config.disabled) {
this.removeClasses(node, 'exit');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  onEntered = (node, appearing) => {
    this.removeClasses(node, 'exit');

    const type = appearing ? 'appear' : 'enter'
    this.removeClasses(node, type);
    this.addClass(node, type, 'done');

    if (this.props.onEntered) {
      this.props.onEntered(node, appearing)
    }
  }

this.addClass(node, 'exit', 'done');

if (config.disabled) {
this.removeClasses(node, 'enter');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  onExited = (node) => {
    this.removeClasses(node, 'enter');
    this.removeClasses(node, 'appear');

    this.removeClasses(node, 'exit');
    this.addClass(node, 'exit', 'done');

    if (this.props.onExited) {
      this.props.onExited(node)
    }
  }

@matt-oconnell
Copy link

@taion @karlwills We're running into this issue as well. Are there any blockers to @taion 's suggested fixes?

I can try and take over the PR if you guys need some more resources on this

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.

3 participants