Skip to content

Commit

Permalink
refactor(ResponsiveContainer): improve performance memoizing internal…
Browse files Browse the repository at this point in the history
… variables
  • Loading branch information
marcalexiei authored and ckifer committed Jan 6, 2023
1 parent fe8f758 commit 3214d14
Showing 1 changed file with 42 additions and 38 deletions.
80 changes: 42 additions & 38 deletions src/component/ResponsiveContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,17 @@
*/
import classNames from 'classnames';
import _ from 'lodash';
import React, { ReactElement, forwardRef, cloneElement, useState, useImperativeHandle, useRef, useEffect } from 'react';
import React, {
ReactElement,
forwardRef,
cloneElement,
useState,
useImperativeHandle,
useRef,
useEffect,
useCallback,
useMemo,
} from 'react';
import ReactResizeDetector from 'react-resize-detector';
import { isPercent } from '../util/DataUtils';
import { warn } from '../util/LogUtils';
Expand All @@ -21,11 +31,6 @@ export interface Props {
className?: string | number;
}

interface State {
containerWidth: number;
containerHeight: number;
}

export const ResponsiveContainer = forwardRef(
(
{
Expand All @@ -42,16 +47,18 @@ export const ResponsiveContainer = forwardRef(
}: Props,
ref,
) => {
const [sizes, setSizes] = useState<State>({
const [sizes, setSizes] = useState<{
containerWidth: number;
containerHeight: number;
}>({
containerWidth: -1,
containerHeight: -1,
});

const containerRef = useRef<HTMLDivElement>(null);
useImperativeHandle(ref, () => containerRef, [containerRef]);

const [mounted, setMounted] = useState<boolean>(false);

const getContainerSize = () => {
const getContainerSize = useCallback(() => {
if (!containerRef.current) {
return null;
}
Expand All @@ -60,28 +67,31 @@ export const ResponsiveContainer = forwardRef(
containerWidth: containerRef.current.clientWidth,
containerHeight: containerRef.current.clientHeight,
};
};

const updateDimensionsImmediate = () => {
if (!mounted) {
return;
}
}, []);

const updateDimensionsImmediate = useCallback(() => {
const newSize = getContainerSize();

if (newSize) {
const { containerWidth: oldWidth, containerHeight: oldHeight } = sizes;
const { containerWidth, containerHeight } = newSize;

if (containerWidth !== oldWidth || containerHeight !== oldHeight) {
setSizes({ containerWidth, containerHeight });
}
setSizes(currentSizes => {
const { containerWidth: oldWidth, containerHeight: oldHeight } = currentSizes;
if (containerWidth !== oldWidth || containerHeight !== oldHeight) {
return { containerWidth, containerHeight };
}

return currentSizes;
});
}
};
}, [getContainerSize]);

const handleResize = debounce > 0 ? _.debounce(updateDimensionsImmediate, debounce) : updateDimensionsImmediate;
const handleResize = useMemo(
() => (debounce > 0 ? _.debounce(updateDimensionsImmediate, debounce) : updateDimensionsImmediate),
[debounce, updateDimensionsImmediate],
);

const renderChart = () => {
const computedSizes = useMemo(() => {
const { containerWidth, containerHeight } = sizes;

if (containerWidth < 0 || containerHeight < 0) {
Expand Down Expand Up @@ -132,27 +142,21 @@ export const ResponsiveContainer = forwardRef(
aspect,
);

return cloneElement(children, {
return {
width: calculatedWidth,
height: calculatedHeight,
});
};
};
}, [aspect, height, maxHeight, minHeight, minWidth, sizes, width]);

useEffect(() => {
if (mounted) {
const size = getContainerSize();
const size = getContainerSize();

if (size) {
setSizes(size);
}
if (size) {
setSizes(size);
}
}, [mounted]);

useEffect(() => {
setMounted(true);
}, []);
}, [getContainerSize]);

const style = { width, height, minWidth, minHeight, maxHeight };
const style: React.CSSProperties = { width, height, minWidth, minHeight, maxHeight };

return (
<ReactResizeDetector handleWidth handleHeight onResize={handleResize} targetRef={containerRef}>
Expand All @@ -162,7 +166,7 @@ export const ResponsiveContainer = forwardRef(
style={style}
ref={containerRef}
>
{renderChart()}
{cloneElement(children, computedSizes)}
</div>
</ReactResizeDetector>
);
Expand Down

4 comments on commit 3214d14

@ckifer
Copy link
Member

@ckifer ckifer commented on 3214d14 Jan 12, 2023

Choose a reason for hiding this comment

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

@marcalexiei - this commit broke how responsive container currently works. We'll need to take another better look at this.

@ckifer
Copy link
Member

@ckifer ckifer commented on 3214d14 Jan 12, 2023

Choose a reason for hiding this comment

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

2.3.1.ResponsiveContainer.mov

@marcalexiei
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch, I've tested it in the ResponsiveContainer demo page and I've seen something like this.
I hope to find a way to be able to reproduce it there.

@ckifer Sorry for the trouble and thanks for the video

@ckifer
Copy link
Member

@ckifer ckifer commented on 3214d14 Jan 12, 2023

Choose a reason for hiding this comment

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

@marcalexiei I was kinda sorta able to repro there but it was a lot more evident in the website itself. Not sure why. In the demos page I still noticed things coming in from the side instead of the bottom.

And no worries, an easy revert

Please sign in to comment.