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.

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] Decouple margin and axis-size #16418

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
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
3 changes: 1 addition & 2 deletions packages/x-charts/src/BarChart/useBarChartProps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { ChartsAxisProps } from '../ChartsAxis';
import { ChartsAxisHighlightProps } from '../ChartsAxisHighlight';
import { ChartsLegendSlotExtension } from '../ChartsLegend';
import type { ChartsWrapperProps } from '../internals/components/ChartsWrapper';
import { calculateMargins } from '../internals/calculateMargins';
import { BAR_CHART_PLUGINS, BarChartPluginsSignatures } from './BarChart.plugins';

/**
Expand Down Expand Up @@ -78,7 +77,7 @@ export const useBarChartProps = (props: BarChartProps) => {
})),
width,
height,
margin: calculateMargins({ margin, hideLegend, slotProps, series }),
margin,
colors,
dataset,
xAxis:
Expand Down
183 changes: 130 additions & 53 deletions packages/x-charts/src/ChartsAxis/ChartsAxis.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,32 +11,38 @@ import {
ChartsYAxisProps,
} from '../models/axis';
import { useXAxes, useYAxes } from '../hooks';
import { DEFAULT_AXIS_SIZE } from '../constants';

// TODO: Add links to the migration docs for each prop
Copy link
Member

Choose a reason for hiding this comment

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

Should this be done in this PR or as a follow-up? Just making sure it doesn't fall through the cracks 😄

export interface ChartsAxisProps {
/**
* Indicate which axis to display the top of the charts.
* Can be a string (the id of the axis) or an object `ChartsXAxisProps`.
* @default null
* @deprecated Use `xAxis[].position="top"` instead.
Copy link
Member

Choose a reason for hiding this comment

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

We're suggesting using xAxis, but xAxis isn't a prop of this component. I'm a bit confused by this, so I suspect a user would be too. Where should I set the xAxis prop to maintain the behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 true it is not a prop of the ChartsAxis but it is of the XxxChart since it reuses the props.

In this use-case I would assume there is no "single best approach", since the prop is not on this component, but also not on a single external component.

What we can do is provide a url to the migration guide where we can explain with specific examples.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm not sure what's the best way to proceed here. I suppose a URL to the migration guide is helpful, at least, so unless we can think of any better option, that seems good to me 👍

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 not super concerned by this point. I guess most of the users are using directly the ChartsXAxis/ChartsYAxis and they all now where the xAxis props goes because with series they are the most used

*/
topAxis?: null | string | ChartsXAxisProps;
/**
* Indicate which axis to display the right of the charts.
* Can be a string (the id of the axis) or an object `ChartsYAxisProps`.
* @default null
* @deprecated Use `yAxis[].position="right"` instead.
*/
rightAxis?: null | string | ChartsYAxisProps;
/**
* Indicate which axis to display the bottom of the charts.
* Can be a string (the id of the axis) or an object `ChartsXAxisProps`.
* @default xAxisIds[0] The id of the first provided axis
* @deprecated Use `xAxis[].position="bottom"` instead.
*/
bottomAxis?: null | string | ChartsXAxisProps;
/**
* Indicate which axis to display the left of the charts.
* Can be a string (the id of the axis) or an object `ChartsYAxisProps`.
* @default yAxisIds[0] The id of the first provided axis
* @deprecated Use `yAxis[].position="left"` instead.
*/
leftAxis?: null | string | ChartsYAxisProps;
/**
* Indicate which axis to display the right of the charts.
* Can be a string (the id of the axis) or an object `ChartsYAxisProps`.
* @default null
*/
rightAxis?: null | string | ChartsYAxisProps;
/**
* Overridable component slots.
* @default {}
Expand All @@ -51,13 +57,12 @@ export interface ChartsAxisProps {

const getAxisId = (
propsValue: undefined | null | AxisId | ChartsXAxisProps | ChartsYAxisProps,
defaultAxisId?: AxisId,
): AxisId | null => {
if (propsValue == null) {
return null;
}
if (typeof propsValue === 'object') {
return propsValue.axisId ?? defaultAxisId ?? null;
return propsValue.axisId ?? null;
}
return propsValue;
};
Expand All @@ -69,7 +74,6 @@ const mergeProps = (
) => {
return typeof axisConfig === 'object'
? {
...axisConfig,
slots: { ...slots, ...axisConfig?.slots },
slotProps: { ...slotProps, ...axisConfig?.slotProps },
}
Expand All @@ -86,58 +90,131 @@ const mergeProps = (
* - [ChartsAxis API](https://mui.com/x/api/charts/charts-axis/)
*/
function ChartsAxis(props: ChartsAxisProps) {
const { topAxis, leftAxis, rightAxis, bottomAxis, slots, slotProps } = props;
const {
topAxis: topAxisProp,
rightAxis: rightAxisProp,
bottomAxis: bottomAxisProp,
leftAxis: leftAxisProp,
slots,
slotProps,
} = props;
const { xAxis, xAxisIds } = useXAxes();
const { yAxis, yAxisIds } = useYAxes();

const leftId = getAxisId(leftAxis === undefined ? yAxisIds[0] : leftAxis, yAxisIds[0]);
const bottomId = getAxisId(bottomAxis === undefined ? xAxisIds[0] : bottomAxis, xAxisIds[0]);
const topId = getAxisId(topAxis, xAxisIds[0]);
const rightId = getAxisId(rightAxis, yAxisIds[0]);
const topId = getAxisId(topAxisProp);
const rightId = getAxisId(rightAxisProp);
const bottomId = getAxisId(bottomAxisProp);
const leftId = getAxisId(leftAxisProp);

if (topId !== null && !xAxis[topId]) {
throw new Error(
[
`MUI X: id used for top axis "${topId}" is not defined.`,
`Available ids are: ${xAxisIds.join(', ')}.`,
].join('\n'),
);
}
if (leftId !== null && !yAxis[leftId]) {
throw new Error(
[
`MUI X: id used for left axis "${leftId}" is not defined.`,
`Available ids are: ${yAxisIds.join(', ')}.`,
].join('\n'),
);
if (process.env.NODE_ENV !== 'production') {
if (topId !== null && !xAxis[topId]) {
throw new Error(
[
`MUI X: id used for top axis "${topId}" is not defined.`,
`Available ids are: ${xAxisIds.join(', ')}.`,
].join('\n'),
);
}
if (rightId !== null && !yAxis[rightId]) {
throw new Error(
[
`MUI X: id used for right axis "${rightId}" is not defined.`,
`Available ids are: ${yAxisIds.join(', ')}.`,
].join('\n'),
);
}
if (bottomId !== null && !xAxis[bottomId]) {
throw new Error(
[
`MUI X: id used for bottom axis "${bottomId}" is not defined.`,
`Available ids are: ${xAxisIds.join(', ')}.`,
].join('\n'),
);
}
if (leftId !== null && !yAxis[leftId]) {
throw new Error(
[
`MUI X: id used for left axis "${leftId}" is not defined.`,
`Available ids are: ${yAxisIds.join(', ')}.`,
].join('\n'),
);
}
}
if (rightId !== null && !yAxis[rightId]) {
throw new Error(
[
`MUI X: id used for right axis "${rightId}" is not defined.`,
`Available ids are: ${yAxisIds.join(', ')}.`,
].join('\n'),
);
}
if (bottomId !== null && !xAxis[bottomId]) {
throw new Error(
[
`MUI X: id used for bottom axis "${bottomId}" is not defined.`,
`Available ids are: ${xAxisIds.join(', ')}.`,
].join('\n'),
);
}
const topAxisProps = mergeProps(topAxis, slots, slotProps);
const bottomAxisProps = mergeProps(bottomAxis, slots, slotProps);
const leftAxisProps = mergeProps(leftAxis, slots, slotProps);
const rightAxisProps = mergeProps(rightAxis, slots, slotProps);

const topAxes = topId
? [xAxis[topId]]
: xAxisIds.map((id) => xAxis[id]).filter((axis) => axis.position === 'top');
const rightAxes = rightId
? [yAxis[rightId]]
: yAxisIds.map((id) => yAxis[id]).filter((axis) => axis.position === 'right');
const bottomAxes = bottomId
? [xAxis[bottomId]]
: xAxisIds.map((id) => xAxis[id]).filter((axis) => axis.position === 'bottom');
const leftAxes = leftId
? [yAxis[leftId]]
: yAxisIds.map((id) => yAxis[id]).filter((axis) => axis.position === 'left');

const completeTopAxisProps = topAxes.map((axis) => ({
...axis,
...mergeProps(axis, slots, slotProps),
}));
const completeRightAxisProps = rightAxes.map((axis) => ({
...axis,
...mergeProps(axis, slots, slotProps),
}));
const completeBottomAxisProps = (
bottomAxes.length === 0 && !xAxis[xAxisIds[0]].position ? [xAxis[xAxisIds[0]]] : bottomAxes
).map((axis) => ({ ...axis, ...mergeProps(axis, slots, slotProps) }));
const completeLeftAxisProps = (
leftAxes.length === 0 && !yAxis[yAxisIds[0]].position ? [yAxis[yAxisIds[0]]] : leftAxes
).map((axis) => ({ ...axis, ...mergeProps(axis, slots, slotProps) }));
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified if you setup by default axis.position === 'bottom' for the first item in axis pluggin


return (
<React.Fragment>
{topId && <ChartsXAxis {...topAxisProps} position="top" axisId={topId} />}
{bottomId && <ChartsXAxis {...bottomAxisProps} position="bottom" axisId={bottomId} />}
{leftId && <ChartsYAxis {...leftAxisProps} position="left" axisId={leftId} />}
{rightId && <ChartsYAxis {...rightAxisProps} position="right" axisId={rightId} />}
{completeTopAxisProps.map((axis, i, arr) => (
<ChartsXAxis
key={axis.id}
{...axis}
position="top"
axisId={axis.id}
offset={arr
.slice(0, i)
.reduce((acc, curr) => acc + (curr.height ?? DEFAULT_AXIS_SIZE), axis.offset ?? 0)}
/>
))}
{completeRightAxisProps.map((axis, i, arr) => (
<ChartsYAxis
key={axis.id}
{...axis}
position="right"
axisId={axis.id}
offset={arr
.slice(0, i)
.reduce((acc, curr) => acc + (curr.width ?? DEFAULT_AXIS_SIZE), axis.offset ?? 0)}
/>
))}
{completeBottomAxisProps.map((axis, i, arr) => (
<ChartsXAxis
key={axis.id}
{...axis}
position="bottom"
axisId={axis.id}
offset={arr
.slice(0, i)
.reduce((acc, curr) => acc + (curr.height ?? DEFAULT_AXIS_SIZE), axis.offset ?? 0)}
/>
))}
{completeLeftAxisProps.map((axis, i, arr) => (
<ChartsYAxis
key={axis.id}
{...axis}
position="left"
axisId={axis.id}
offset={arr
.slice(0, i)
.reduce((acc, curr) => acc + (curr.width ?? DEFAULT_AXIS_SIZE), axis.offset ?? 0)}
/>
))}
</React.Fragment>
);
}
Expand Down
66 changes: 37 additions & 29 deletions packages/x-charts/src/ChartsXAxis/ChartsXAxis.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ const defaultProps = {
disableLine: false,
disableTicks: false,
tickSize: 6,
offset: 0,
} as const;

/**
Expand Down Expand Up @@ -136,6 +137,7 @@ function ChartsXAxis(inProps: ChartsXAxisProps) {
tickPlacement,
tickLabelPlacement,
sx,
offset,
} = defaultizedProps;

const theme = useTheme();
Expand Down Expand Up @@ -212,44 +214,50 @@ function ChartsXAxis(inProps: ChartsXAxisProps) {
}
return (
<XAxisRoot
transform={`translate(0, ${position === 'bottom' ? top + height : top})`}
transform={`translate(0, ${position === 'bottom' ? top + height + offset : top - offset})`}
className={classes.root}
sx={sx}
>
{!disableLine && (
<Line x1={left} x2={left + width} className={classes.line} {...slotProps?.axisLine} />
)}

{xTicksWithDimension.map(({ formattedValue, offset, labelOffset, skipLabel }, index) => {
const xTickLabel = labelOffset ?? 0;
const yTickLabel = positionSign * (tickSize + 3);
{xTicksWithDimension.map(
({ formattedValue, offset: tickOffset, labelOffset, skipLabel }, index) => {
const xTickLabel = labelOffset ?? 0;
const yTickLabel = positionSign * (tickSize + 3);

const showTick = instance.isPointInside({ x: offset, y: -1 }, { direction: 'x' });
const showTickLabel = instance.isPointInside(
{ x: offset + xTickLabel, y: -1 },
{ direction: 'x' },
);
return (
<g key={index} transform={`translate(${offset}, 0)`} className={classes.tickContainer}>
{!disableTicks && showTick && (
<Tick
y2={positionSign * tickSize}
className={classes.tick}
{...slotProps?.axisTick}
/>
)}
const showTick = instance.isPointInside({ x: tickOffset, y: -1 }, { direction: 'x' });
const showTickLabel = instance.isPointInside(
{ x: tickOffset + xTickLabel, y: -1 },
{ direction: 'x' },
);
return (
<g
key={index}
transform={`translate(${tickOffset}, 0)`}
className={classes.tickContainer}
>
{!disableTicks && showTick && (
<Tick
y2={positionSign * tickSize}
className={classes.tick}
{...slotProps?.axisTick}
/>
)}

{formattedValue !== undefined && !skipLabel && showTickLabel && (
<TickLabel
x={xTickLabel}
y={yTickLabel}
{...axisTickLabelProps}
text={formattedValue.toString()}
/>
)}
</g>
);
})}
{formattedValue !== undefined && !skipLabel && showTickLabel && (
<TickLabel
x={xTickLabel}
y={yTickLabel}
{...axisTickLabelProps}
text={formattedValue.toString()}
/>
)}
</g>
);
},
)}

{label && (
<g className={classes.label}>
Expand Down
Loading
Loading