From 1e5ea4f1abac4dbc888fb9b9341700f420583410 Mon Sep 17 00:00:00 2001 From: Nolan Kaplan Date: Thu, 11 Jun 2020 17:47:49 -0400 Subject: [PATCH 1/8] add items before the fierst new key if there are any --- src/utils/ChildMapping.js | 16 +++++++++++++++- test/CSSTransitionGroup-test.js | 12 ++++++------ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/utils/ChildMapping.js b/src/utils/ChildMapping.js index 60a360d9..b6363578 100644 --- a/src/utils/ChildMapping.js +++ b/src/utils/ChildMapping.js @@ -48,6 +48,15 @@ export function mergeChildMappings(prev, next) { let nextKeysPending = Object.create(null) let pendingKeys = [] + + let firstNewKey + for (let nextKey in next) { + if (!(nextKey in prev)) { + firstNewKey = nextKey + break + } + } + for (let prevKey in prev) { if (prevKey in next) { if (pendingKeys.length) { @@ -59,6 +68,11 @@ export function mergeChildMappings(prev, next) { } } + if (firstNewKey && pendingKeys.length) { + nextKeysPending[firstNewKey] = pendingKeys + pendingKeys = [] + } + let i let childMapping = {} for (let nextKey in next) { @@ -73,7 +87,7 @@ export function mergeChildMappings(prev, next) { childMapping[nextKey] = getValueForKey(nextKey) } - // Finally, add the keys which didn't appear before any key in `next` + // Finally, add the keys which didn't appear before any key in `next` if there were no new keys added for (i = 0; i < pendingKeys.length; i++) { childMapping[pendingKeys[i]] = getValueForKey(pendingKeys[i]) } diff --git a/test/CSSTransitionGroup-test.js b/test/CSSTransitionGroup-test.js index 371981d9..4c6085aa 100644 --- a/test/CSSTransitionGroup-test.js +++ b/test/CSSTransitionGroup-test.js @@ -65,8 +65,8 @@ describe('CSSTransitionGroup', () => { ); expect(transitionGroupDiv.childNodes.length).toBe(2); - expect(transitionGroupDiv.childNodes[0].id).toBe('two'); - expect(transitionGroupDiv.childNodes[1].id).toBe('one'); + expect(transitionGroupDiv.childNodes[0].id).toBe('one'); + expect(transitionGroupDiv.childNodes[1].id).toBe('two'); jest.runAllTimers(); @@ -98,8 +98,8 @@ describe('CSSTransitionGroup', () => { ); expect(transitionGroupDiv.childNodes.length).toBe(2); - expect(transitionGroupDiv.childNodes[0].id).toBe('two'); - expect(transitionGroupDiv.childNodes[1].id).toBe('one'); + expect(transitionGroupDiv.childNodes[0].id).toBe('one'); + expect(transitionGroupDiv.childNodes[1].id).toBe('two'); }); it('should switch transitionLeave from false to true', () => { @@ -133,8 +133,8 @@ describe('CSSTransitionGroup', () => { ); expect(transitionGroupDiv.childNodes.length).toBe(2); - expect(transitionGroupDiv.childNodes[0].id).toBe('three'); - expect(transitionGroupDiv.childNodes[1].id).toBe('two'); + expect(transitionGroupDiv.childNodes[0].id).toBe('two'); + expect(transitionGroupDiv.childNodes[1].id).toBe('three'); }); From fa494cb36922a0ee0879111c7a243040c444aabe Mon Sep 17 00:00:00 2001 From: Nolan Kaplan Date: Thu, 11 Jun 2020 19:38:33 -0400 Subject: [PATCH 2/8] use first new key after last prev key --- src/utils/ChildMapping.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/utils/ChildMapping.js b/src/utils/ChildMapping.js index b6363578..f210b7ce 100644 --- a/src/utils/ChildMapping.js +++ b/src/utils/ChildMapping.js @@ -49,11 +49,14 @@ export function mergeChildMappings(prev, next) { let pendingKeys = [] - let firstNewKey + let firstNewKeyAfterLastPrevKey for (let nextKey in next) { if (!(nextKey in prev)) { - firstNewKey = nextKey - break + if (!firstNewKeyAfterLastPrevKey) { + firstNewKeyAfterLastPrevKey = nextKey; + } + } else { + firstNewKeyAfterLastPrevKey = undefined; } } @@ -68,8 +71,8 @@ export function mergeChildMappings(prev, next) { } } - if (firstNewKey && pendingKeys.length) { - nextKeysPending[firstNewKey] = pendingKeys + if (firstNewKeyAfterLastPrevKey && pendingKeys.length) { + nextKeysPending[firstNewKeyAfterLastPrevKey] = pendingKeys pendingKeys = [] } From 91e3bcea20c9b3f2f81cb8e8ff1d91fbb9aaf5a2 Mon Sep 17 00:00:00 2001 From: Nolan Kaplan Date: Thu, 11 Jun 2020 19:41:48 -0400 Subject: [PATCH 3/8] remove semicolon --- src/utils/ChildMapping.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/ChildMapping.js b/src/utils/ChildMapping.js index f210b7ce..830a9255 100644 --- a/src/utils/ChildMapping.js +++ b/src/utils/ChildMapping.js @@ -53,10 +53,10 @@ export function mergeChildMappings(prev, next) { for (let nextKey in next) { if (!(nextKey in prev)) { if (!firstNewKeyAfterLastPrevKey) { - firstNewKeyAfterLastPrevKey = nextKey; + firstNewKeyAfterLastPrevKey = nextKey } } else { - firstNewKeyAfterLastPrevKey = undefined; + firstNewKeyAfterLastPrevKey = undefined } } From 8ca7852c5b2de3c3751303c4b538b067dbaeacca Mon Sep 17 00:00:00 2001 From: Nolan Kaplan Date: Thu, 11 Jun 2020 19:46:44 -0400 Subject: [PATCH 4/8] update comment --- src/utils/ChildMapping.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/ChildMapping.js b/src/utils/ChildMapping.js index 830a9255..f6bd1e03 100644 --- a/src/utils/ChildMapping.js +++ b/src/utils/ChildMapping.js @@ -90,7 +90,7 @@ export function mergeChildMappings(prev, next) { childMapping[nextKey] = getValueForKey(nextKey) } - // Finally, add the keys which didn't appear before any key in `next` if there were no new keys added + // Finally, add the keys which didn't appear before any key in `next` if there were no new keys added at the end for (i = 0; i < pendingKeys.length; i++) { childMapping[pendingKeys[i]] = getValueForKey(pendingKeys[i]) } From e714415d3e40954e272b0ae15b02a4d98c93bc02 Mon Sep 17 00:00:00 2001 From: Nolan Kaplan Date: Thu, 11 Jun 2020 19:57:33 -0400 Subject: [PATCH 5/8] optimize, only look for new keys if there are pending keys left --- src/utils/ChildMapping.js | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/utils/ChildMapping.js b/src/utils/ChildMapping.js index f6bd1e03..a58718cd 100644 --- a/src/utils/ChildMapping.js +++ b/src/utils/ChildMapping.js @@ -49,17 +49,6 @@ export function mergeChildMappings(prev, next) { let pendingKeys = [] - let firstNewKeyAfterLastPrevKey - for (let nextKey in next) { - if (!(nextKey in prev)) { - if (!firstNewKeyAfterLastPrevKey) { - firstNewKeyAfterLastPrevKey = nextKey - } - } else { - firstNewKeyAfterLastPrevKey = undefined - } - } - for (let prevKey in prev) { if (prevKey in next) { if (pendingKeys.length) { @@ -71,9 +60,23 @@ export function mergeChildMappings(prev, next) { } } - if (firstNewKeyAfterLastPrevKey && pendingKeys.length) { - nextKeysPending[firstNewKeyAfterLastPrevKey] = pendingKeys - pendingKeys = [] + // If there are any pending keys left, check if there are new keys that they should be in front of + if (pendingKeys.length) { + let firstNewKeyAfterLastPrevKey + for (let nextKey in next) { + if (!(nextKey in prev)) { + if (!firstNewKeyAfterLastPrevKey) { + firstNewKeyAfterLastPrevKey = nextKey + } + } else { + firstNewKeyAfterLastPrevKey = undefined + } + } + + if (firstNewKeyAfterLastPrevKey) { + nextKeysPending[firstNewKeyAfterLastPrevKey] = pendingKeys + pendingKeys = [] + } } let i From f76a297f93719e552a3b1ef0089390af5da96c3d Mon Sep 17 00:00:00 2001 From: Nolan Kaplan Date: Thu, 11 Jun 2020 20:01:20 -0400 Subject: [PATCH 6/8] delete stray whitespace --- src/utils/ChildMapping.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/utils/ChildMapping.js b/src/utils/ChildMapping.js index a58718cd..2be27ef1 100644 --- a/src/utils/ChildMapping.js +++ b/src/utils/ChildMapping.js @@ -48,7 +48,6 @@ export function mergeChildMappings(prev, next) { let nextKeysPending = Object.create(null) let pendingKeys = [] - for (let prevKey in prev) { if (prevKey in next) { if (pendingKeys.length) { From a6151d47ad445893a544f8410be0612455ac38d0 Mon Sep 17 00:00:00 2001 From: Nolan Kaplan Date: Mon, 15 Jun 2020 15:11:00 -0400 Subject: [PATCH 7/8] add appendOnReplace prop instead --- src/Transition.js | 11 +++ src/utils/ChildMapping.js | 104 +++++++++++++++++++-------- test/CSSTransitionGroup-test.js | 121 +++++++++++++++++++++++++++++--- 3 files changed, 198 insertions(+), 38 deletions(-) diff --git a/src/Transition.js b/src/Transition.js index 981acc26..94190642 100644 --- a/src/Transition.js +++ b/src/Transition.js @@ -364,6 +364,7 @@ class Transition extends React.Component { onExiting: _onExiting, onExited: _onExited, nodeRef: _nodeRef, + appendOnReplace: _appendOnReplace, ...childProps } = this.props @@ -561,6 +562,16 @@ Transition.propTypes = { * @type Function(node: HtmlElement) -> void */ onExited: PropTypes.func, + + /** + * Hint to tell whether the node should be placed before or after the pending items it replaces. + * Everything with appendOnReplace set to true will be placed after the removed keys, + * while everything else goes before the removed keys. + * + * **Note**: applies when replacing one or more consecutive existing items with one or more consecutive new items. + * + */ + appendOnReplace: PropTypes.bool, } // Name the function so it is clearer in the documentation diff --git a/src/utils/ChildMapping.js b/src/utils/ChildMapping.js index 2be27ef1..1521de08 100644 --- a/src/utils/ChildMapping.js +++ b/src/utils/ChildMapping.js @@ -18,6 +18,22 @@ export function getChildMapping(children, mapFn) { return result } +/** + * Given `this.props.children`, return an object mapping key to child's appendOnReplace prop. + * + * @param {*} children `this.props.children` + * @return {object} Mapping of key to placement hint + */ +function getChildHintMapping(children) { + let result = Object.create(null) + if (children) + Children.map(children, c => c).forEach(child => { + // run the map function here instead so that the key is the computed one + if (isValidElement(child)) result[child.key] = child.props && child.props.appendOnReplace + }) + return result +} + /** * When you're adding or removing children some may be added or removed in the * same render pass. We want to show *both* since we want to simultaneously @@ -32,12 +48,14 @@ export function getChildMapping(children, mapFn) { * `ReactTransitionChildMapping.getChildMapping()`. * @param {object} next next children as returned from * `ReactTransitionChildMapping.getChildMapping()`. + * @param {object} appendHints placement hint mapping as returned from getChildHintMapping(). * @return {object} a key set that contains all keys in `prev` and all keys * in `next` in a reasonable order. */ -export function mergeChildMappings(prev, next) { +export function mergeChildMappings(prev, next, appendHints) { prev = prev || {} next = next || {} + appendHints = appendHints || {} function getValueForKey(key) { return key in next ? next[key] : prev[key] @@ -59,42 +77,67 @@ export function mergeChildMappings(prev, next) { } } - // If there are any pending keys left, check if there are new keys that they should be in front of - if (pendingKeys.length) { - let firstNewKeyAfterLastPrevKey - for (let nextKey in next) { - if (!(nextKey in prev)) { - if (!firstNewKeyAfterLastPrevKey) { - firstNewKeyAfterLastPrevKey = nextKey + let pendingNewKeys = [] + let prependKeys = [] + let appendKeys = [] + let childMapping = {} + let i + for (let nextKey in next) { + if (nextKey in prev) { + if (nextKeysPending[nextKey]) { + prependKeys = [] + appendKeys = [] + if (pendingNewKeys.length) { + // If there were pending new keys that replaced the nextKeysPending, + // place them before or after the prevKeys based on the value of their appendOnReplace prop + for (i = 0; i < pendingNewKeys.length; i++) { + if (!appendHints[pendingNewKeys[i]]) { + prependKeys.push(pendingNewKeys[i]) + } else { + appendKeys.push(pendingNewKeys[i]) + } + } + pendingNewKeys = [] + } + const combinedPendingKeys = prependKeys.concat(nextKeysPending[nextKey]).concat(appendKeys) + for (i = 0; i < combinedPendingKeys.length; i++) { + let pendingNextKey = combinedPendingKeys[i] + childMapping[pendingNextKey] = getValueForKey( + pendingNextKey + ) + } + } else if (pendingNewKeys.length) { + // If there were no pending prevKeys, place the pendingNewKeys before nextKey + for (i = 0; i < pendingNewKeys.length; i++) { + childMapping[pendingNewKeys[i]] = getValueForKey(pendingNewKeys[i]) } - } else { - firstNewKeyAfterLastPrevKey = undefined + pendingNewKeys = [] } - } - - if (firstNewKeyAfterLastPrevKey) { - nextKeysPending[firstNewKeyAfterLastPrevKey] = pendingKeys - pendingKeys = [] + childMapping[nextKey] = getValueForKey(nextKey) + } else { + // If the key is new, wait to see if they go before or after any pending keys + pendingNewKeys.push(nextKey) } } - let i - let childMapping = {} - for (let nextKey in next) { - if (nextKeysPending[nextKey]) { - for (i = 0; i < nextKeysPending[nextKey].length; i++) { - let pendingNextKey = nextKeysPending[nextKey][i] - childMapping[nextKeysPending[nextKey][i]] = getValueForKey( - pendingNextKey - ) - } + // For the remaining pending and new keys that didn't appear before any keys in next, + // place the new keys before or after the pending keys based on the value of their appendOnReplace prop + prependKeys = [] + appendKeys = [] + const finalAppendKeys = [] + for (i = 0; i < pendingNewKeys.length; i++) { + if (!appendHints[pendingNewKeys[i]]) { + prependKeys.push(pendingNewKeys[i]) + } else { + appendKeys.push(pendingNewKeys[i]) } - childMapping[nextKey] = getValueForKey(nextKey) } - // Finally, add the keys which didn't appear before any key in `next` if there were no new keys added at the end - for (i = 0; i < pendingKeys.length; i++) { - childMapping[pendingKeys[i]] = getValueForKey(pendingKeys[i]) + const finalPendingKeys = prependKeys.concat(pendingKeys).concat(appendKeys) + + // Finally, add the remaining keys + for (i = 0; i < finalPendingKeys.length; i++) { + childMapping[finalPendingKeys[i]] = getValueForKey(finalPendingKeys[i]) } return childMapping @@ -118,7 +161,8 @@ export function getInitialChildMapping(props, onExited) { export function getNextChildMapping(nextProps, prevChildMapping, onExited) { let nextChildMapping = getChildMapping(nextProps.children) - let children = mergeChildMappings(prevChildMapping, nextChildMapping) + let nextChildHintMapping = getChildHintMapping(nextProps.children) + let children = mergeChildMappings(prevChildMapping, nextChildMapping, nextChildHintMapping) Object.keys(children).forEach(key => { let child = children[key] diff --git a/test/CSSTransitionGroup-test.js b/test/CSSTransitionGroup-test.js index 4c6085aa..d6579ed1 100644 --- a/test/CSSTransitionGroup-test.js +++ b/test/CSSTransitionGroup-test.js @@ -48,7 +48,7 @@ describe('CSSTransitionGroup', () => { it('should clean-up silently after the timeout elapses', () => { render( - + , container, ); @@ -59,14 +59,14 @@ describe('CSSTransitionGroup', () => { render( - + , container, ); expect(transitionGroupDiv.childNodes.length).toBe(2); - expect(transitionGroupDiv.childNodes[0].id).toBe('one'); - expect(transitionGroupDiv.childNodes[1].id).toBe('two'); + expect(transitionGroupDiv.childNodes[0].id).toBe('two'); + expect(transitionGroupDiv.childNodes[1].id).toBe('one'); jest.runAllTimers(); @@ -98,8 +98,8 @@ describe('CSSTransitionGroup', () => { ); expect(transitionGroupDiv.childNodes.length).toBe(2); - expect(transitionGroupDiv.childNodes[0].id).toBe('one'); - expect(transitionGroupDiv.childNodes[1].id).toBe('two'); + expect(transitionGroupDiv.childNodes[0].id).toBe('two'); + expect(transitionGroupDiv.childNodes[1].id).toBe('one'); }); it('should switch transitionLeave from false to true', () => { @@ -133,10 +133,115 @@ describe('CSSTransitionGroup', () => { ); expect(transitionGroupDiv.childNodes.length).toBe(2); - expect(transitionGroupDiv.childNodes[0].id).toBe('two'); - expect(transitionGroupDiv.childNodes[1].id).toBe('three'); + expect(transitionGroupDiv.childNodes[0].id).toBe('three'); + expect(transitionGroupDiv.childNodes[1].id).toBe('two'); + }); + + it('should place new node after the node it replaces when appendOnReplace is true', () => { + render( + + + , + container, + ); + + const transitionGroupDiv = container.childNodes[0] + + expect(transitionGroupDiv.childNodes.length).toBe(1); + + render( + + + , + container, + ); + + expect(transitionGroupDiv.childNodes.length).toBe(2); + expect(transitionGroupDiv.childNodes[0].id).toBe('one'); + expect(transitionGroupDiv.childNodes[1].id).toBe('two'); + }); + + it('should place new node after last pending node when appendOnReplace is true', () => { + render( + + + + , + container, + ); + + const transitionGroupDiv = container.childNodes[0] + + render( + + + + , + container, + ); + + expect(transitionGroupDiv.childNodes.length).toBe(3); + expect(transitionGroupDiv.childNodes[1].id).toBe('two'); + expect(transitionGroupDiv.childNodes[2].id).toBe('three'); }); + it('should place new node after the pending node in the middle of the list when appendOnReplace is true', () => { + render( + + + + + , + container, + ); + + const transitionGroupDiv = container.childNodes[0] + + render( + + + + + , + container, + ); + + expect(transitionGroupDiv.childNodes.length).toBe(4); + expect(transitionGroupDiv.childNodes[0].id).toBe('one'); + expect(transitionGroupDiv.childNodes[1].id).toBe('two'); + expect(transitionGroupDiv.childNodes[2].id).toBe('four'); + expect(transitionGroupDiv.childNodes[3].id).toBe('three'); + }); + + it('should place pending nodes without appendOnReplace first, then new nodes, then pending nodes with appendOnReplace last', () => { + render( + + + + + , + container, + ); + + const transitionGroupDiv = container.childNodes[0] + + render( + + + + + + , + container, + ); + + expect(transitionGroupDiv.childNodes.length).toBe(5); + expect(transitionGroupDiv.childNodes[0].id).toBe('one'); + expect(transitionGroupDiv.childNodes[1].id).toBe('four'); + expect(transitionGroupDiv.childNodes[2].id).toBe('two'); + expect(transitionGroupDiv.childNodes[3].id).toBe('five'); + expect(transitionGroupDiv.childNodes[4].id).toBe('three'); + }); it('should work with a null child', () => { render( From b71119fbed3cdbdf98b460e22ff2b3cbec9579b1 Mon Sep 17 00:00:00 2001 From: Nolan Kaplan Date: Mon, 15 Jun 2020 15:27:59 -0400 Subject: [PATCH 8/8] lint, only reorder final set if both new and existing have items --- src/utils/ChildMapping.js | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/utils/ChildMapping.js b/src/utils/ChildMapping.js index 1521de08..77976f9e 100644 --- a/src/utils/ChildMapping.js +++ b/src/utils/ChildMapping.js @@ -124,17 +124,22 @@ export function mergeChildMappings(prev, next, appendHints) { // place the new keys before or after the pending keys based on the value of their appendOnReplace prop prependKeys = [] appendKeys = [] - const finalAppendKeys = [] - for (i = 0; i < pendingNewKeys.length; i++) { - if (!appendHints[pendingNewKeys[i]]) { - prependKeys.push(pendingNewKeys[i]) - } else { - appendKeys.push(pendingNewKeys[i]) + let finalPendingKeys = [] + if (pendingKeys.length && pendingNewKeys.length) { + for (i = 0; i < pendingNewKeys.length; i++) { + if (!appendHints[pendingNewKeys[i]]) { + prependKeys.push(pendingNewKeys[i]) + } else { + appendKeys.push(pendingNewKeys[i]) + } } + finalPendingKeys = prependKeys.concat(pendingKeys).concat(appendKeys) + } else if (pendingKeys.length) { + finalPendingKeys = pendingKeys + } else if (pendingNewKeys.length) { + finalPendingKeys = pendingNewKeys } - const finalPendingKeys = prependKeys.concat(pendingKeys).concat(appendKeys) - // Finally, add the remaining keys for (i = 0; i < finalPendingKeys.length; i++) { childMapping[finalPendingKeys[i]] = getValueForKey(finalPendingKeys[i])