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.

Sign up for GitHub

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

[charts] Fix LineChart animation being stuck with initial drawing area value #14553

Merged
merged 6 commits into from
Sep 16, 2024
Merged

[charts] Fix LineChart animation being stuck with initial drawing area value #14553

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
365ab9f
Fix animation stuck with initial drawing area value
JCQuintas Sep 9, 2024
c644426
Improve initial render behavior when size is unknown
JCQuintas Sep 9, 2024
681f0dd
Merge commit 'dd98382e7b1657235c12c1334e44a68fa003bd4a' into improve-…
JCQuintas Sep 9, 2024
82f2aff
Merge commit '149cecda61c257795d56fa323d56e10cc1d93e21' into improve-…
JCQuintas Sep 16, 2024
e426eec
Add prop to control resolve size
JCQuintas Sep 16, 2024
cb7058e
scripts
JCQuintas Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions packages/x-charts/src/LineChart/AnimatedLine.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,21 @@ export interface AnimatedLineProps extends React.ComponentPropsWithoutRef<'path'
*/
function AnimatedLine(props: AnimatedLineProps) {
const { d, skipAnimation, ownerState, ...other } = props;
const { left, top, bottom, width, height, right } = useDrawingArea();
const drawingArea = useDrawingArea();
const chartId = useChartId();

const stringInterpolator = useStringInterpolator(d);

const transitionAppear = useTransition([1], {
from: { animatedWidth: left },
to: { animatedWidth: width + left + right },
enter: { animatedWidth: width + left + right },
leave: { animatedWidth: left },
reset: false,
immediate: skipAnimation,
});
const transitionAppear = useTransition<typeof drawingArea, { animatedWidth: number }>(
[drawingArea],
{
from: (v) => ({ animatedWidth: v.left }),
enter: (v) => ({ animatedWidth: v.width + v.left + v.right }),
leave: (v) => ({ animatedWidth: v.width + v.left + v.right }),
reset: false,
immediate: skipAnimation,
},
);

const transitionChange = useTransition([stringInterpolator], {
from: { value: 0 },
Expand All @@ -74,7 +76,12 @@ function AnimatedLine(props: AnimatedLineProps) {
<React.Fragment>
<clipPath id={clipId}>
{transitionAppear((style) => (
<animated.rect x={0} y={0} width={style.animatedWidth} height={top + height + bottom} />
<animated.rect
x={0}
y={0}
width={style.animatedWidth}
height={drawingArea.top + drawingArea.height + drawingArea.bottom}
/>
))}
</clipPath>
<g clipPath={`url(#${clipId})`}>
Expand Down
33 changes: 26 additions & 7 deletions packages/x-charts/src/ResponsiveChartContainer/useChartContainerDimensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import useEnhancedEffect from '@mui/utils/useEnhancedEffect';
import ownerWindow from '@mui/utils/ownerWindow';

export const useChartContainerDimensions = (inWidth?: number, inHeight?: number) => {
const displayError = React.useRef<boolean>(false);
const stateRef = React.useRef({ displayError: false, initialCompute: true, computeRun: 0 });
const rootRef = React.useRef(null);

const [width, setWidth] = React.useState(0);
Expand All @@ -14,7 +14,7 @@ export const useChartContainerDimensions = (inWidth?: number, inHeight?: number)
const mainEl = rootRef?.current;

if (!mainEl) {
return;
return {};
}

const win = ownerWindow(mainEl);
Expand All @@ -25,13 +25,32 @@ export const useChartContainerDimensions = (inWidth?: number, inHeight?: number)

setWidth(newWidth);
setHeight(newHeight);

return { width: newWidth, height: newHeight };
}, []);

React.useEffect(() => {
// Ensure the error detection occurs after the first rendering.
displayError.current = true;
stateRef.current.displayError = true;
}, []);

// This effect is used to compute the size of the container on the initial render.
// It is not bound to the raf loop to avoid an unwanted "resize" event.
// https://github.com/mui/mui-x/issues/13477#issuecomment-2336634785
useEnhancedEffect(() => {
// computeRun is used to avoid infinite loops.
if (!stateRef.current.initialCompute || stateRef.current.computeRun > 20) {
return;
}

const computedSize = computeSize();
if (computedSize.width !== width || computedSize.height !== height) {
stateRef.current.computeRun += 1;
} else if (stateRef.current.initialCompute) {
stateRef.current.initialCompute = false;
}
}, [width, height, computeSize]);
Copy link
Member

Choose a reason for hiding this comment

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

Could you describe the behavior with for example what is currently happening step by step, and how this PR modify it?

I don't huderstand how adding a useEnhancedEffect could help to prevent inifint loop.

If you want a nice reproduction of an infinit loop there is one here:
#12263 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

This useEffect doesn't prevent an infinite loop. Only computeRun > 20 does. It serves as a circuit breaker in case the layout never settles into a defined size, which would mean this useEffect would run indefinitely.

This useEffect only happens before/during the first render, when the page layout is figuring out what size the components need. Hence it is different from the issues in your example, where the loop in on the render side.

To fix the loop in your example, we can try to handle it on the useEnhancedEffect that has the ResizeObserver inside. Though I'm not sure how useful it would be, since my current fix is only for layouts that will definitely settle into a defined width/height, but your example will never settle 😓

Explanation of what it does

The main idea of this useEnhancedEffect is that everything in it will happen before we see anything, since it is running on the layout effect and committing changes to the state, the state will update synchronously.

The state updates until it settles on a size (new size is the same as last one) or it runs more than 20 times (usually it only requires 5-6 runs from what I tested).

Once one of these two conditions are met, the hook stops looping (width/height stop changing), and the first actual react render happens.

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with the solution. But this will silently delay the painting without warning the developers about the improvement they could bring. We should add at least a warning if computeRun > 2

We can not do computeRun > 0 because the react strict mode trigger twice the useLayoutEffect. Si the ref is updated twice before the state get updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had added a warning at first but thought it could be unnecessary and removed it 😅

Will add it back

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum, I actually removed it because it is an actual valid behaviour that wouldn't require a warning. 🤔

Like, if the user uses this inside a grid, this will always warn, even though a grid's width would eventually settle in the screen's width.

Copy link
Member

Choose a reason for hiding this comment

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

it is an actual valid behaviour

I disagree on this point. For me the valid behavior is "The container has a defined size". This PR is adding a nice fallback

I tried with Recharts and they have the same behavior as us currently.

Capture.video.du.2024-09-16.10-02-01.mp4

From my point of view it would be bad to have such loop without informing user since it silently fix something the developers could potentially do better.

In addition to a warning, it could be a warning plus a prop to silence the warning. Or no warning and a prop to activate the feature

Copy link
Member Author

@JCQuintas JCQuintas Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As "valid behaviour" I mean it is something the browser does on its own. Not that our behaviour is valid 😅

I would agree with a prop for this though.

Something like:

 /**
  * The chart will try to wait for the parent container to resolve its size
  * before it renders for the first time.
  * 
  * This can be useful in some scenarios where the chart appear to grow after
  * the first render, like when used inside a grid.
  */
  resolveSizeBeforeRender


useEnhancedEffect(() => {
if (inWidth !== undefined && inHeight !== undefined) {
return () => {};
Expand Down Expand Up @@ -67,17 +86,17 @@ export const useChartContainerDimensions = (inWidth?: number, inHeight?: number)
}, [computeSize, inHeight, inWidth]);

if (process.env.NODE_ENV !== 'production') {
if (displayError.current && inWidth === undefined && width === 0) {
if (stateRef.current.displayError && inWidth === undefined && width === 0) {
console.error(
`MUI X: ChartContainer does not have \`width\` prop, and its container has no \`width\` defined.`,
);
displayError.current = false;
stateRef.current.displayError = false;
}
if (displayError.current && inHeight === undefined && height === 0) {
if (stateRef.current.displayError && inHeight === undefined && height === 0) {
console.error(
`MUI X: ChartContainer does not have \`height\` prop, and its container has no \`height\` defined.`,
);
displayError.current = false;
stateRef.current.displayError = false;
}
}

Expand Down