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] Remove intrinsic size requirement #15471

Merged
merged 14 commits into from
Nov 19, 2024
Merged

[charts] Remove intrinsic size requirement #15471

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0d7a441
Try removing intrinsic size requirement
JCQuintas Nov 18, 2024
5c258c0
Merge branch 'master' into chart-remove-intrinsic-size-requirement
JCQuintas Nov 18, 2024
a0e960a
prevent changes
JCQuintas Nov 18, 2024
f620c87
Update packages/x-charts/src/ChartContainer/ResizableContainer.tsx
JCQuintas Nov 18, 2024
9f62e38
try direct render
JCQuintas Nov 18, 2024
b110fd0
keep bool inside context
JCQuintas Nov 18, 2024
13a6ce8
remove unused props
JCQuintas Nov 18, 2024
38e2229
Revert "remove unused props"
JCQuintas Nov 18, 2024
c282408
Revert "Revert "remove unused props""
JCQuintas Nov 19, 2024
6ff7a2d
update scripts
JCQuintas Nov 19, 2024
cf12e27
Update packages/x-charts/src/context/SizeProvider/useChartContainerDi…
JCQuintas Nov 19, 2024
aee3572
Add BC
JCQuintas Nov 19, 2024
ac93bca
Merge branch 'master' into chart-remove-intrinsic-size-requirement
JCQuintas Nov 19, 2024
3cae24c
Update docs/data/migration/migration-charts-v7/migration-charts-v7.md
JCQuintas Nov 19, 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
4 changes: 2 additions & 2 deletions packages/x-charts/src/ChartContainer/ResizableContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ export const ResizableContainerRoot = styled('div', {
* @ignore - do not document.
*/
function ResizableContainer(props: { children: React.ReactNode }) {
const { inHeight, inWidth, hasIntrinsicSize, containerRef } = useSize();
const { inHeight, inWidth, containerRef } = useSize();

return (
<ResizableContainerRoot
{...props}
ownerState={{ width: inWidth, height: inHeight }}
ref={containerRef}
>
{hasIntrinsicSize && props.children}
{props.children}
Copy link
Member

Choose a reason for hiding this comment

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

That let me think that the initial idea of this early return was to skip providers on first render.

If you have no width/height, you will have default width/height used to compute scales, coordinate, ... And when it renders, the width/height get their real size and everything run a second time. The idea was to skip the useless step.

Copy link
Member Author

Choose a reason for hiding this comment

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

Providers are now on top of this component, since the datasource is the <ChartDataProvider> now. So I moved the check to the ChartsSurface. This means we can render the <svg/> TAG on the first render, but everything else is only rendered if we have an intrinsic size

</ResizableContainerRoot>
);
}
Expand Down
6 changes: 3 additions & 3 deletions packages/x-charts/src/ChartsSurface/ChartsSurface.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { useAxisEvents } from '../hooks/useAxisEvents';
import { ChartsAxesGradients } from '../internals/components/ChartsAxesGradients';
import { useDrawingArea } from '../hooks';
import { useSurfaceRef } from '../context/SvgRefProvider';
import { useSize } from '../context/SizeProvider';

export interface ChartsSurfaceProps {
className?: string;
Expand Down Expand Up @@ -36,6 +37,7 @@ const ChartsSurface = React.forwardRef<SVGSVGElement, ChartsSurfaceProps>(functi
ref: React.Ref<SVGSVGElement>,
) {
const { width, height, left, right, top, bottom } = useDrawingArea();
const { hasIntrinsicSize } = useSize();
const surfaceRef = useSurfaceRef();
const handleRef = useForkRef(surfaceRef, ref);
const themeProps = useThemeProps({ props: inProps, name: 'MuiChartsSurface' });
Expand All @@ -56,8 +58,6 @@ const ChartsSurface = React.forwardRef<SVGSVGElement, ChartsSurfaceProps>(functi

return (
<ChartsSurfaceStyles
width={svgWidth}
height={svgHeight}
viewBox={`${svgView.x} ${svgView.y} ${svgView.width} ${svgView.height}`}
className={className}
{...other}
Expand All @@ -66,7 +66,7 @@ const ChartsSurface = React.forwardRef<SVGSVGElement, ChartsSurfaceProps>(functi
{title && <title>{title}</title>}
{desc && <desc>{desc}</desc>}
<ChartsAxesGradients />
{children}
{hasIntrinsicSize && children}
</ChartsSurfaceStyles>
);
});
Expand Down
6 changes: 1 addition & 5 deletions packages/x-charts/src/context/SizeProvider/SizeProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@ import { useChartContainerDimensions } from './useChartContainerDimensions';
* This provider is also responsible for resolving the size of the container before rendering and if the parent size changes.
*/
function SizeProvider(props: SizeProviderProps) {
const dimensions = useChartContainerDimensions(
props.width,
props.height,
props.resolveSizeBeforeRender,
);
const dimensions = useChartContainerDimensions(props.width, props.height);

const value = React.useMemo(
() => ({
Expand Down
16 changes: 6 additions & 10 deletions packages/x-charts/src/context/SizeProvider/useChartContainerDimensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,9 @@ import * as React from 'react';
import useEnhancedEffect from '@mui/utils/useEnhancedEffect';
import ownerWindow from '@mui/utils/ownerWindow';

export const useChartContainerDimensions = (
inWidth?: number,
inHeight?: number,
resolveSizeBeforeRender?: boolean,
) => {
const MAX_COMPUTE_RUN = 10;

export const useChartContainerDimensions = (inWidth?: number, inHeight?: number) => {
const hasInSize = inWidth !== undefined && inHeight !== undefined;
const stateRef = React.useRef({ displayError: false, initialCompute: true, computeRun: 0 });
const rootRef = React.useRef<HTMLDivElement>(null);
Expand Down Expand Up @@ -48,9 +46,8 @@ export const useChartContainerDimensions = (
// computeRun is used to avoid infinite loops.
if (
hasInSize ||
!resolveSizeBeforeRender ||
!stateRef.current.initialCompute ||
Comment on lines 48 to 49
Copy link
Member

Choose a reason for hiding this comment

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

If you don't need the resolveSizeBeforeRender you can remove it from the props. The PR change migth impact much more files with the scripts ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, had the commit ready, but there would be a lot of file changes

Do you think we need a codemod? I don't think a lot of people would be using it 🙃

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a BC to changelog/migration docs

Copy link
Member

Choose a reason for hiding this comment

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

Should be enough

stateRef.current.computeRun > 20
stateRef.current.computeRun > MAX_COMPUTE_RUN
) {
return;
}
Expand All @@ -61,7 +58,7 @@ export const useChartContainerDimensions = (
} else if (stateRef.current.initialCompute) {
stateRef.current.initialCompute = false;
}
}, [width, height, computeSize, resolveSizeBeforeRender, hasInSize]);
}, [width, height, computeSize, hasInSize]);

useEnhancedEffect(() => {
if (hasInSize) {
Expand Down Expand Up @@ -111,15 +108,14 @@ export const useChartContainerDimensions = (
stateRef.current.displayError = false;
}
}

const finalWidth = inWidth ?? width;
const finalHeight = inHeight ?? height;

return {
containerRef: rootRef,
width: finalWidth,
height: finalHeight,
hasIntrinsicSize: Boolean(finalWidth && finalHeight),
hasIntrinsicSize: Boolean(finalWidth > 0 && finalHeight > 0),
JCQuintas marked this conversation as resolved.
Show resolved Hide resolved
inWidth,
inHeight,
};
Expand Down