From 6536804cfddcfd8010b6a3930348e9dc96ef52e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Burzy=C5=84ski?= Date: Thu, 9 May 2019 14:02:06 +0200 Subject: [PATCH 1/5] Add failing test for not removing dynamically applied classes --- test/CSSTransition-test.js | 80 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/test/CSSTransition-test.js b/test/CSSTransition-test.js index e5436c52..5715faa9 100644 --- a/test/CSSTransition-test.js +++ b/test/CSSTransition-test.js @@ -2,6 +2,7 @@ import React from 'react'; import { mount } from 'enzyme'; import CSSTransition from '../src/CSSTransition'; +import TransitionGroup from '../src/TransitionGroup'; describe('CSSTransition', () => { @@ -334,4 +335,83 @@ describe('CSSTransition', () => { }); }); }); + + describe('reentering', () => { + it('should remove dynamically applied classes', done => { + let count = 0; + class Test extends React.Component { + render() { + const { direction, text, ...props } = this.props; + + return ( + + React.cloneElement(child, { + classNames: direction + }) + } + > + + {text} + + + ) + } + } + + const instance = mount() + + const rerender = getProps => new Promise(resolve => + instance.setProps({ + onEnter: undefined, + onEntering: undefined, + onEntered: undefined, + onExit: undefined, + onExiting: undefined, + onExited: undefined, + ...getProps(resolve) + }) + ); + + Promise.resolve().then(() => + rerender(resolve => ({ + direction: 'up', + text: 'bar', + + onEnter(node) { + count++; + expect(node.className).toEqual('up-enter'); + }, + onEntering(node) { + count++; + expect(node.className).toEqual('up-enter up-enter-active'); + resolve() + } + })) + ).then(() => { + return rerender(resolve => ({ + direction: 'down', + text: 'foo', + + onEntering(node) { + count++; + expect(node.className).toEqual('down-enter down-enter-active'); + }, + onEntered(node) { + count++; + expect(node.className).toEqual('down-enter-done'); + resolve(); + } + })) + }).then(() => { + expect(count).toEqual(4); + done(); + }); + }); + }); }); From 87cf56c9acd449527936798c9e5cd10c7c9e7a53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Burzy=C5=84ski?= Date: Thu, 9 May 2019 14:02:44 +0200 Subject: [PATCH 2/5] Fixed issue with not removing dynamically applied classes --- src/CSSTransition.js | 91 ++++++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 42 deletions(-) diff --git a/src/CSSTransition.js b/src/CSSTransition.js index 9cc2543e..af6268cb 100644 --- a/src/CSSTransition.js +++ b/src/CSSTransition.js @@ -74,11 +74,16 @@ class CSSTransition extends React.Component { static defaultProps = { classNames: '' } - onEnter = (node, appearing) => { - const { className } = this.getClassNames(appearing ? 'appear' : 'enter') + appliedClasses = { + appear: {}, + enter: {}, + exit: {}, + } + + onEnter = (node, appearing) => { this.removeClasses(node, 'exit'); - addClass(node, className) + this.addClass(node, appearing ? 'appear' : 'enter'); if (this.props.onEnter) { this.props.onEnter(node, appearing) @@ -86,11 +91,8 @@ class CSSTransition extends React.Component { } onEntering = (node, appearing) => { - const { activeClassName } = this.getClassNames( - appearing ? 'appear' : 'enter' - ); - - this.reflowAndAddClass(node, activeClassName) + const type = appearing ? 'appear' : 'enter'; + this.addClass(node, type, 'active') if (this.props.onEntering) { this.props.onEntering(node, appearing) @@ -98,14 +100,9 @@ class CSSTransition extends React.Component { } onEntered = (node, appearing) => { - const appearClassName = this.getClassNames('appear').doneClassName; - const enterClassName = this.getClassNames('enter').doneClassName; - const doneClassName = appearing - ? `${appearClassName} ${enterClassName}` - : enterClassName; - - this.removeClasses(node, appearing ? 'appear' : 'enter'); - addClass(node, doneClassName); + const type = appearing ? 'appear' : 'enter' + this.removeClasses(node, type); + this.addClass(node, type, 'done'); if (this.props.onEntered) { this.props.onEntered(node, appearing) @@ -113,11 +110,9 @@ class CSSTransition extends React.Component { } onExit = (node) => { - const { className } = this.getClassNames('exit') - this.removeClasses(node, 'appear'); this.removeClasses(node, 'enter'); - addClass(node, className) + this.addClass(node, 'exit') if (this.props.onExit) { this.props.onExit(node) @@ -125,9 +120,7 @@ class CSSTransition extends React.Component { } onExiting = (node) => { - const { activeClassName } = this.getClassNames('exit') - - this.reflowAndAddClass(node, activeClassName) + this.addClass(node, 'exit', 'active') if (this.props.onExiting) { this.props.onExiting(node) @@ -135,10 +128,8 @@ class CSSTransition extends React.Component { } onExited = (node) => { - const { doneClassName } = this.getClassNames('exit'); - this.removeClasses(node, 'exit'); - addClass(node, doneClassName); + this.addClass(node, 'exit', 'done'); if (this.props.onExited) { this.props.onExited(node) @@ -150,44 +141,60 @@ class CSSTransition extends React.Component { const isStringClassNames = typeof classNames === 'string'; const prefix = isStringClassNames && classNames ? classNames + '-' : ''; - let className = isStringClassNames ? + let baseClassName = isStringClassNames ? prefix + type : classNames[type] let activeClassName = isStringClassNames ? - className + '-active' : classNames[type + 'Active']; + baseClassName + '-active' : classNames[type + 'Active']; let doneClassName = isStringClassNames ? - className + '-done' : classNames[type + 'Done']; + baseClassName + '-done' : classNames[type + 'Done']; return { - className, + baseClassName, activeClassName, doneClassName }; } - removeClasses(node, type) { - const { className, activeClassName, doneClassName } = this.getClassNames(type) - className && removeClass(node, className); - activeClassName && removeClass(node, activeClassName); - doneClassName && removeClass(node, doneClassName); - } + addClass(node, type, phase = 'base') { + let className = this.getClassNames(type)[`${phase}ClassName`]; + + if (type === 'appear' && phase === 'done') { + className += ` ${this.getClassNames('enter').doneClassName}`; + } - reflowAndAddClass(node, className) { // This is for to force a repaint, // which is necessary in order to transition styles when adding a class name. - if (className) { + if (phase === 'active') { /* eslint-disable no-unused-expressions */ node && node.scrollTop; - /* eslint-enable no-unused-expressions */ - addClass(node, className); } + + this.appliedClasses[type][phase] = className + addClass(node, className) } - render() { - const props = { ...this.props }; + removeClasses(node, type) { + const { + base: baseClassName, + active: activeClassName, + done: doneClassName + } = this.appliedClasses[type] + + if (baseClassName) { + removeClass(node, baseClassName); + } + if (activeClassName) { + removeClass(node, activeClassName); + } + if (doneClassName) { + removeClass(node, doneClassName); + } + } - delete props.classNames; + render() { + const { classNames: _, ...props } = this.props; return ( Date: Thu, 9 May 2019 14:11:12 +0200 Subject: [PATCH 3/5] Clear cached applied classes when removing them --- src/CSSTransition.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/CSSTransition.js b/src/CSSTransition.js index af6268cb..eebfa392 100644 --- a/src/CSSTransition.js +++ b/src/CSSTransition.js @@ -182,6 +182,8 @@ class CSSTransition extends React.Component { done: doneClassName } = this.appliedClasses[type] + this.appliedClasses[type] = {}; + if (baseClassName) { removeClass(node, baseClassName); } From 1635b5b3da2a704edb3439b4a63dc0f40767080f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Burzy=C5=84ski?= Date: Thu, 9 May 2019 17:32:57 +0200 Subject: [PATCH 4/5] Few stylistic changes suggested at code review --- src/CSSTransition.js | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/CSSTransition.js b/src/CSSTransition.js index eebfa392..06bcd4e8 100644 --- a/src/CSSTransition.js +++ b/src/CSSTransition.js @@ -83,7 +83,7 @@ class CSSTransition extends React.Component { onEnter = (node, appearing) => { this.removeClasses(node, 'exit'); - this.addClass(node, appearing ? 'appear' : 'enter'); + this.addClass(node, appearing ? 'appear' : 'enter', 'base'); if (this.props.onEnter) { this.props.onEnter(node, appearing) @@ -112,7 +112,7 @@ class CSSTransition extends React.Component { onExit = (node) => { this.removeClasses(node, 'appear'); this.removeClasses(node, 'enter'); - this.addClass(node, 'exit') + this.addClass(node, 'exit','base') if (this.props.onExit) { this.props.onExit(node) @@ -139,16 +139,21 @@ class CSSTransition extends React.Component { getClassNames = (type) => { const { classNames } = this.props; const isStringClassNames = typeof classNames === 'string'; - const prefix = isStringClassNames && classNames ? classNames + '-' : ''; + const prefix = isStringClassNames && classNames + ? `${classNames}-` + : ''; - let baseClassName = isStringClassNames ? - prefix + type : classNames[type] + let baseClassName = isStringClassNames + ? `${prefix}${type}` + : classNames[type] - let activeClassName = isStringClassNames ? - baseClassName + '-active' : classNames[type + 'Active']; + let activeClassName = isStringClassNames + ? `${baseClassName}-active` + : classNames[`${type}Active`]; - let doneClassName = isStringClassNames ? - baseClassName + '-done' : classNames[type + 'Done']; + let doneClassName = isStringClassNames + ? `${baseClassName}-done` + : classNames[`${type}Done`]; return { baseClassName, @@ -157,7 +162,7 @@ class CSSTransition extends React.Component { }; } - addClass(node, type, phase = 'base') { + addClass(node, type, phase) { let className = this.getClassNames(type)[`${phase}ClassName`]; if (type === 'appear' && phase === 'done') { From e980bd0401d68a5022f11201c3c94106f1fb2c30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Burzy=C5=84ski?= Date: Thu, 9 May 2019 20:34:03 +0200 Subject: [PATCH 5/5] Fix linting errors --- src/CSSTransition.js | 2 +- test/.eslintrc.yml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/CSSTransition.js b/src/CSSTransition.js index 06bcd4e8..f0cd415c 100644 --- a/src/CSSTransition.js +++ b/src/CSSTransition.js @@ -112,7 +112,7 @@ class CSSTransition extends React.Component { onExit = (node) => { this.removeClasses(node, 'appear'); this.removeClasses(node, 'enter'); - this.addClass(node, 'exit','base') + this.addClass(node, 'exit', 'base') if (this.props.onExit) { this.props.onExit(node) diff --git a/test/.eslintrc.yml b/test/.eslintrc.yml index 33d5b6c5..374695a7 100644 --- a/test/.eslintrc.yml +++ b/test/.eslintrc.yml @@ -1,6 +1,7 @@ env: jest: true + es6: true rules: no-require: off global-require: off