From 9a8fa038a4542edc8a959313e68a0aa339b1b45b Mon Sep 17 00:00:00 2001 From: Dan McGrath Date: Thu, 12 Dec 2024 09:02:58 -0500 Subject: [PATCH 1/2] chore: comments explaining animation bug --- .../src/victory-transition/victory-transition.tsx | 6 ++++++ packages/victory-pie/src/victory-pie.tsx | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/victory-core/src/victory-transition/victory-transition.tsx b/packages/victory-core/src/victory-transition/victory-transition.tsx index 47b863908..0709c193e 100644 --- a/packages/victory-core/src/victory-transition/victory-transition.tsx +++ b/packages/victory-core/src/victory-transition/victory-transition.tsx @@ -157,6 +157,12 @@ export class VictoryTransition extends React.Component< if (!this.state) { return this.props; } + // This causes the following bug in victory pie: + // If nodes exit, we run the exit transition using the old props. + // This causes us to overwrite any new data values with old data values. + // For example, if oldProps has data: [1, 2, 3, 4, 6] and newProps has data: [5, 2, 3, 4], + // we end up with data: [1, 2, 3, 4, 0] after the exit transition (default exit transition for pie chart is + // before: () => ({ _y: 0 }), which causes the existing nodes to be set to 0). return this.state.nodesWillExit ? this.state.oldProps || this.props : this.props; diff --git a/packages/victory-pie/src/victory-pie.tsx b/packages/victory-pie/src/victory-pie.tsx index ad92dbdf6..39932cd0b 100644 --- a/packages/victory-pie/src/victory-pie.tsx +++ b/packages/victory-pie/src/victory-pie.tsx @@ -117,7 +117,7 @@ class VictoryPieBase extends React.Component { duration: 500, before: () => ({ _y: 0, label: " " }), after: (datum) => ({ - y_: datum._y, + _y: datum._y, label: datum.label, }), }, From 1a8eafe934d8b752aed7efb4dc649b27ec367125 Mon Sep 17 00:00:00 2001 From: Dan McGrath Date: Fri, 13 Dec 2024 15:11:38 -0500 Subject: [PATCH 2/2] fix: stale data transformed during exit transition Partially resolves the issue of victory-pie animations not working as expected when nodes exit. Issue persists when nodes enter again. --- .../victory-animation/victory-animation.tsx | 2 +- .../victory-transition/victory-transition.tsx | 10 +- .../src/victory-util/transitions.ts | 137 +++++++++++++++--- .../victory-pie/animation.stories.tsx | 76 ++++++++++ 4 files changed, 198 insertions(+), 27 deletions(-) create mode 100644 stories/victory-charts/victory-pie/animation.stories.tsx diff --git a/packages/victory-core/src/victory-animation/victory-animation.tsx b/packages/victory-core/src/victory-animation/victory-animation.tsx index 318c8c73a..5390168e7 100644 --- a/packages/victory-core/src/victory-animation/victory-animation.tsx +++ b/packages/victory-core/src/victory-animation/victory-animation.tsx @@ -1,4 +1,4 @@ -import React from "react"; +import React, { useEffect } from "react"; import * as d3Ease from "victory-vendor/d3-ease"; import { victoryInterpolator } from "./util"; import TimerContext from "../victory-util/timer-context"; diff --git a/packages/victory-core/src/victory-transition/victory-transition.tsx b/packages/victory-core/src/victory-transition/victory-transition.tsx index 0709c193e..f8702c39b 100644 --- a/packages/victory-core/src/victory-transition/victory-transition.tsx +++ b/packages/victory-core/src/victory-transition/victory-transition.tsx @@ -195,8 +195,14 @@ export class VictoryTransition extends React.Component< const props = this.pickProps(); const getTransitionProps = this.props.animate?.getTransitions ? this.props.animate.getTransitions - : Transitions.getTransitionPropsFactory(props, this.state, (newState) => - this.setState(newState), + : Transitions.getTransitionPropsFactory( + props, + this.state, + (newState) => this.setState(newState), + { + oldProps: this.state.oldProps, + nextProps: this.props, + }, ); const child = React.Children.toArray( props.children, diff --git a/packages/victory-core/src/victory-util/transitions.ts b/packages/victory-core/src/victory-util/transitions.ts index ab80a1cf5..a9e27ac88 100644 --- a/packages/victory-core/src/victory-util/transitions.ts +++ b/packages/victory-core/src/victory-util/transitions.ts @@ -45,12 +45,15 @@ function getNodeTransitions(oldData, nextData) { const oldDataKeyed = oldData && getKeyedData(oldData); const nextDataKeyed = nextData && getKeyedData(nextData); - return { + const result = { entering: oldDataKeyed && getKeyedDataDifference(nextDataKeyed, oldDataKeyed), exiting: nextDataKeyed && getKeyedDataDifference(oldDataKeyed, nextDataKeyed), }; + // Debugging + console.log("getNodeTransitions: ", result); + return result; } function getChildData(child) { @@ -182,6 +185,7 @@ function getChildPropsOnExit( data, exitingNodes, cb, + { oldProps, nextProps }, ): TransitionProps { // Whether or not _this_ child has exiting nodes, we want the exit- // transition for all children to have the same duration, delay, etc. @@ -189,6 +193,13 @@ function getChildPropsOnExit( const newAnimate = Object.assign({}, animate, onExit); let newData = data; + // Debugging + console.group("getChildPropsOnExit"); + console.log(oldProps.children.props.data); + console.log(newData); + console.log(nextProps.children.props.data); + console.groupEnd(); + if (exitingNodes) { // After the exit transition occurs, trigger the animations for // nodes that are neither exiting nor entering. @@ -197,18 +208,43 @@ function getChildPropsOnExit( animate.onExit && animate.onExit.before ? animate.onExit.before : identity; - // If nodes need to exit, transform them with the provided onExit.before function. - newData = data.map((datum, idx) => { - const key = (datum.key || idx).toString(); - return exitingNodes[key] - ? Object.assign({}, datum, before(datum, idx, data)) - : datum; - }); + // + if (oldProps && nextProps) { + const transformedPreviousData = transformChildData( + oldProps.children.props.data, + exitingNodes, + before, + ); + // Merge the incoming data with the transformed existing data + newData = defaults( + [], + nextProps.children.props.data, + transformedPreviousData, + ); + } else { + // If nodes need to exit, transform them with the provided onExit.before function. + // TODO: refactor using `transformChildData` + newData = data.map((datum, idx) => { + const key = (datum.key || idx).toString(); + return exitingNodes[key] + ? Object.assign({}, datum, before(datum, idx, data)) + : datum; + }); + } } return { animate: newAnimate, data: newData }; } +function transformChildData(data, nodes, transform) { + return data.map((datum, idx) => { + const key = (datum.key || idx).toString(); + return nodes[key] + ? Object.assign({}, datum, transform(datum, idx, data)) + : datum; + }); +} + // eslint-disable-next-line max-params function getChildPropsBeforeEnter( animate, @@ -216,9 +252,18 @@ function getChildPropsBeforeEnter( data, enteringNodes, cb, + { oldProps, nextProps }, ): TransitionProps { let newAnimate = animate; let newData = data; + + console.group("getChildPropsBeforeEnter"); + console.log(oldProps); + console.log(oldProps?.children.props.data); + console.log(nextProps?.children.props.data); + console.log(child); + console.groupEnd(); + if (enteringNodes) { // Perform a normal animation here, except - when it finishes - trigger // the transition for entering nodes. @@ -227,15 +272,30 @@ function getChildPropsBeforeEnter( animate.onEnter && animate.onEnter.before ? animate.onEnter.before : identity; - // We want the entering nodes to be included in the transition target - // domain. However, we may not want these nodes to be displayed initially, - // so perform the `onEnter.before` transformation on each node. - newData = data.map((datum, idx) => { - const key = (datum.key || idx).toString(); - return enteringNodes[key] - ? Object.assign({}, datum, before(datum, idx, data)) - : datum; - }); + + if (oldProps && nextProps) { + const transformedPreviousData = transformChildData( + oldProps.children.props.data, + enteringNodes, + before, + ); + newData = defaults( + [], + nextProps.children.props.data, + transformedPreviousData, + ); + } else { + // We want the entering nodes to be included in the transition target + // domain. However, we may not want these nodes to be displayed initially, + // so perform the `onEnter.before` transformation on each node. + // TODO: refactor using `transformChildData` + newData = data.map((datum, idx) => { + const key = (datum.key || idx).toString(); + return enteringNodes[key] + ? Object.assign({}, datum, before(datum, idx, data)) + : datum; + }); + } } return { animate: newAnimate, data: newData }; @@ -291,13 +351,22 @@ function getChildPropsOnEnter( * * @return {Function} Child-prop transformation function. */ -export function getTransitionPropsFactory(props, state, setState) { +export function getTransitionPropsFactory( + props, + state, + setState, + { + oldProps = undefined, + nextProps = undefined, + }: { oldProps?: any; nextProps?: any } = {}, +) { const nodesWillExit = state && state.nodesWillExit; const nodesWillEnter = state && state.nodesWillEnter; const nodesShouldEnter = state && state.nodesShouldEnter; const nodesShouldLoad = state && state.nodesShouldLoad; const nodesDoneLoad = state && state.nodesDoneLoad; const childrenTransitions = (state && state.childrenTransitions) || []; + console.log({ childrenTransitions }); const transitionDurations = { enter: props.animate && props.animate.onEnter && props.animate.onEnter.duration, @@ -322,9 +391,16 @@ export function getTransitionPropsFactory(props, state, setState) { // eslint-disable-next-line max-params const onExit = (nodes, child, data, animate) => { - return getChildPropsOnExit(animate, child, data, nodes, () => { - setState({ nodesWillExit: false }); - }); + return getChildPropsOnExit( + animate, + child, + data, + nodes, + () => { + setState({ nodesWillExit: false }); + }, + { oldProps, nextProps }, + ); }; // eslint-disable-next-line max-params @@ -335,9 +411,16 @@ export function getTransitionPropsFactory(props, state, setState) { }); } - return getChildPropsBeforeEnter(animate, child, data, nodes, () => { - setState({ nodesShouldEnter: true }); - }); + return getChildPropsBeforeEnter( + animate, + child, + data, + nodes, + () => { + setState({ nodesShouldEnter: true }); + }, + { oldProps, nextProps }, + ); }; const getChildTransitionDuration = function (child, type) { @@ -385,6 +468,10 @@ export function getTransitionPropsFactory(props, state, setState) { defaultTransitions && defaultTransitions.onLoad, ); + // It's possible to run into a situation where a transition has nodes that will enter + // but because we're keeping track of this in state, we end up with stale transition state. + // e.g. if a previous transition had nodes that exited, but the next has nodes that enter, we still + // run a transition as if nodes are exiting, which can then transform the data and we end up in a bad state. const childTransitions = childrenTransitions[index] || childrenTransitions[0]; if (!nodesDoneLoad) { @@ -403,6 +490,8 @@ export function getTransitionPropsFactory(props, state, setState) { : getChildTransitionDuration(child, "onExit"); // if nodesWillExit, but this child has no exiting nodes, set a delay instead of a duration const animation = exitingNodes ? { duration: exit } : { delay: exit }; + // Debugging + console.log("calling onExit: ", exitingNodes); return onExit( exitingNodes, child, diff --git a/stories/victory-charts/victory-pie/animation.stories.tsx b/stories/victory-charts/victory-pie/animation.stories.tsx new file mode 100644 index 000000000..3ce35a9a8 --- /dev/null +++ b/stories/victory-charts/victory-pie/animation.stories.tsx @@ -0,0 +1,76 @@ +import React, { useState } from "react"; +import type { Meta } from "@storybook/react"; + +import { VictoryPie, VictoryTheme } from "@/victory"; +import { Story, ComponentMeta } from "./config"; + +const meta: Meta = { + ...ComponentMeta, + title: "Victory Charts/VictoryPie", +}; + +const AFTER_VALUES = [ + { x: 1, y: 9 }, + { x: 2, y: 1 }, + { x: 3, y: 1 }, + { x: 4, y: 1 }, +]; + +const INITIAL_VALUES = [ + { x: 1, y: 1 }, + { x: 2, y: 1 }, + { x: 3, y: 1 }, + { x: 4, y: 1 }, + { x: 5, y: 5 }, + { x: 6, y: 2 }, +]; + +const App = (props) => { + const [data, setData] = useState(INITIAL_VALUES); + + const handleUpdate = () => { + setData(AFTER_VALUES); + }; + + const handleReset = () => { + setData(INITIAL_VALUES); + }; + + return ( + <> +
+ + + + +
+ + {/* */} + + ); +}; + +export const Test: Story = { + args: {}, + render: (props) => ( + <> + + + ), +}; + +export default meta;