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] Make useChartGradientId public #16106

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions packages/x-charts/src/ChartsLegend/ContinuousColorLegend.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
ContinuousColorLegendClasses,
useUtilityClasses,
} from './continuousColorLegendClasses';
import { useChartGradientObjectBound } from '../internals/components/ChartsAxesGradients';
import { useChartGradientIdObjectBoundBuilder } from '../hooks/useChartGradientId';

type LabelFormatter = (params: { value: number | Date; formattedValue: string }) => string;

Expand Down Expand Up @@ -210,7 +210,7 @@ const ContinuousColorLegend = consumeThemeProps(
...other
} = props;

const generateGradientId = useChartGradientObjectBound();
const generateGradientId = useChartGradientIdObjectBoundBuilder();
const axisItem = useAxis({ axisDirection, axisId });

const colorMap = axisItem?.colorMap;
Expand Down Expand Up @@ -261,7 +261,7 @@ const ContinuousColorLegend = consumeThemeProps(
rotate={rotateGradient}
reverse={reverse}
thickness={thickness}
gradientId={gradientId ?? generateGradientId(axisItem.id, axisDirection!)}
gradientId={gradientId ?? generateGradientId(axisItem.id)}
Copy link
Member Author

Choose a reason for hiding this comment

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

@alexfauquette I've removed direction=(x,y,z) from usage. It didn't look useful, as axisId should technically be unique by itself.

Copy link
Member

Choose a reason for hiding this comment

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

The axisId should technically be unique by itself

It's a debate I got when creating the lib. Should axis id be unique in general or unique per direction.

For now, we only enforce the "unique per direction" (If not respected users will get a warning). If you remove the axis-driection in the id, users could get weird resultsaxis-direction without any error in the console.

Duplicated ids are a bit tricky to identify as bug. That's why I preferred to be conservative on it. But that's up to debate

Copy link
Member Author

Choose a reason for hiding this comment

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

Feels a bit useless, id is generally expected to be unique. Although you could argue that axis direction is a different domain 🙃.

Should we hold onto this while we wait for the plugins to merge? then I can try to reason with how we currently handle the warnings ad make a decision 😆

Copy link
Member

Choose a reason for hiding this comment

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

Although you could argue that axis direction is a different domain 🙃

That's exactly my argument :p The main usecase I've in mind is a scatter plot where you can any data grid column versus any other column.

Then the chart has the same set of x and y-axes to chose from. And so either we or the developer have to be careful about avoiding id colision.

By order of preferences, we can

  1. Enforce id unicity
  2. Force adding x/y prefix
  3. Hope user have unique id

So I agree about waiting for the plugins to be merged and verify if we can enforce id unicity

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no warning for duplicate axis ids, only series ids 🫠

      <LineChart
        xAxis={[
          {
            id: 'qwerty',
            data: [10, 20, 30, 40, 50],
            scaleType: 'point',
          },
          {
            id: 'qwerty',
            data: [10, 20, 30, 40, 50],
            scaleType: 'point',
          },
        ]}
        yAxis={[
          {
            id: 'qwerty',
            data: [10, 20, 30, 40, 50],
            scaleType: 'linear',
          },
          {
            id: 'qwerty',
            data: [10, 20, 30, 40, 50],
            scaleType: 'linear',
          },
        ]}
        bottomAxis={{
          tickLabelStyle: {
            angle: 0,
            textAnchor: 'middle',
          },
        }}
        grid={{ vertical: true, horizontal: true }}
        series={[
          {
            id: 'qwerty',
            data: [100, 400, 20, 500, 60],
            xAxisId: 'qwerty',
            yAxisId: 'qwerty',
          },
          // {
          //   id: 'qwerty',
          //   data: [100, 400, 20, 500, 60],
          //   xAxisId: 'qwerty',
          //   yAxisId: 'qwerty',
          // },
        ]}
        height={500}
      />

/>
</li>
{reverse ? minComponent : maxComponent}
Expand Down
19 changes: 9 additions & 10 deletions packages/x-charts/src/LineChart/AreaPlot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@ import { getValueToPositionMapper } from '../hooks/useScale';
import getCurveFactory from '../internals/getCurve';
import { DEFAULT_X_AXIS_KEY } from '../constants';
import { LineItemIdentifier } from '../models/seriesType/line';
import { useChartGradient } from '../internals/components/ChartsAxesGradients';
import { useLineSeries } from '../hooks/useSeries';
import { AxisId } from '../models/axis';
import { useSkipAnimation } from '../context/AnimationProvider';
import { useChartGradientIdBuilder } from '../hooks/useChartGradientId';
import { useXAxes, useYAxes } from '../hooks/useAxis';

export interface AreaPlotSlots extends AreaElementSlots {}
Expand Down Expand Up @@ -52,6 +51,7 @@ const useAggregatedData = () => {
const seriesData = useLineSeries();
const { xAxis, xAxisIds } = useXAxes();
const { yAxis, yAxisIds } = useYAxes();
const getGradientId = useChartGradientIdBuilder();

// This memo prevents odd line chart behavior when hydrating.
const allData = React.useMemo(() => {
Expand Down Expand Up @@ -81,9 +81,9 @@ const useAggregatedData = () => {
const yScale = yAxis[yAxisId].scale;
const xData = xAxis[xAxisId].data;

const gradientUsed: [AxisId, 'x' | 'y'] | undefined =
(yAxis[yAxisId].colorScale && [yAxisId, 'y']) ||
(xAxis[xAxisId].colorScale && [xAxisId, 'x']) ||
const gradientId: string | undefined =
(yAxis[yAxisId].colorScale && getGradientId(yAxisId)) ||
(xAxis[xAxisId].colorScale && getGradientId(xAxisId)) ||
undefined;

if (process.env.NODE_ENV !== 'production') {
Expand Down Expand Up @@ -137,13 +137,13 @@ const useAggregatedData = () => {
const d = areaPath.curve(curve)(d3Data) || '';
return {
...series[seriesId],
gradientUsed,
gradientId,
d,
seriesId,
};
});
});
}, [seriesData, xAxisIds, yAxisIds, xAxis, yAxis]);
}, [seriesData, xAxisIds, yAxisIds, xAxis, yAxis, getGradientId]);

return allData;
};
Expand All @@ -163,20 +163,19 @@ function AreaPlot(props: AreaPlotProps) {
const { slots, slotProps, onItemClick, skipAnimation: inSkipAnimation, ...other } = props;
const skipAnimation = useSkipAnimation(inSkipAnimation);

const getGradientId = useChartGradient();
const completedData = useAggregatedData();

return (
<AreaPlotRoot {...other}>
{completedData.map(
({ d, seriesId, color, area, gradientUsed }) =>
({ d, seriesId, color, area, gradientId }) =>
!!area && (
<AreaElement
key={seriesId}
id={seriesId}
d={d}
color={color}
gradientId={gradientUsed && getGradientId(...gradientUsed)}
gradientId={gradientId}
slots={slots}
slotProps={slotProps}
onClick={onItemClick && ((event) => onItemClick(event, { type: 'line', seriesId }))}
Expand Down
19 changes: 9 additions & 10 deletions packages/x-charts/src/LineChart/LinePlot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@ import { getValueToPositionMapper } from '../hooks/useScale';
import getCurveFactory from '../internals/getCurve';
import { DEFAULT_X_AXIS_KEY } from '../constants';
import { LineItemIdentifier } from '../models/seriesType/line';
import { useChartGradient } from '../internals/components/ChartsAxesGradients';
import { useLineSeries } from '../hooks/useSeries';
import { AxisId } from '../models/axis';
import { useSkipAnimation } from '../context/AnimationProvider';
import { useChartGradientIdBuilder } from '../hooks/useChartGradientId';
import { useXAxes, useYAxes } from '../hooks';

export interface LinePlotSlots extends LineElementSlots {}
Expand Down Expand Up @@ -53,6 +52,7 @@ const useAggregatedData = () => {

const { xAxis, xAxisIds } = useXAxes();
const { yAxis, yAxisIds } = useYAxes();
const getGradientId = useChartGradientIdBuilder();

// This memo prevents odd line chart behavior when hydrating.
const allData = React.useMemo(() => {
Expand All @@ -78,9 +78,9 @@ const useAggregatedData = () => {
const yScale = yAxis[yAxisId].scale;
const xData = xAxis[xAxisId].data;

const gradientUsed: [AxisId, 'x' | 'y'] | undefined =
(yAxis[yAxisId].colorScale && [yAxisId, 'y']) ||
(xAxis[xAxisId].colorScale && [xAxisId, 'x']) ||
const gradientId: string | undefined =
(yAxis[yAxisId].colorScale && getGradientId(yAxisId)) ||
(xAxis[xAxisId].colorScale && getGradientId(xAxisId)) ||
undefined;

if (process.env.NODE_ENV !== 'production') {
Expand Down Expand Up @@ -116,13 +116,13 @@ const useAggregatedData = () => {
const d = linePath.curve(getCurveFactory(series[seriesId].curve))(d3Data) || '';
return {
...series[seriesId],
gradientUsed,
gradientId,
d,
seriesId,
};
});
});
}, [seriesData, xAxisIds, yAxisIds, xAxis, yAxis]);
}, [seriesData, xAxisIds, yAxisIds, xAxis, yAxis, getGradientId]);

return allData;
};
Expand All @@ -141,18 +141,17 @@ function LinePlot(props: LinePlotProps) {
const { slots, slotProps, skipAnimation: inSkipAnimation, onItemClick, ...other } = props;
const skipAnimation = useSkipAnimation(inSkipAnimation);

const getGradientId = useChartGradient();
const completedData = useAggregatedData();
return (
<LinePlotRoot {...other}>
{completedData.map(({ d, seriesId, color, gradientUsed }) => {
{completedData.map(({ d, seriesId, color, gradientId }) => {
return (
<LineElement
key={seriesId}
id={seriesId}
d={d}
color={color}
gradientId={gradientUsed && getGradientId(...gradientUsed)}
gradientId={gradientId}
skipAnimation={skipAnimation}
slots={slots}
slotProps={slotProps}
Expand Down
4 changes: 2 additions & 2 deletions packages/x-charts/src/SparkLineChart/SparkLineChart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { MakeOptional } from '@mui/x-internals/types';
import { BarPlot } from '../BarChart';
import { LinePlot, AreaPlot, LineHighlightPlot } from '../LineChart';
import { ChartContainer, ChartContainerProps } from '../ChartContainer';
import { DEFAULT_X_AXIS_KEY } from '../constants';
import { DEFAULT_X_AXIS_KEY, DEFAULT_Y_AXIS_KEY } from '../constants';
import { ChartsTooltip } from '../ChartsTooltip';
import { ChartsTooltipSlots, ChartsTooltipSlotProps } from '../ChartsTooltip/ChartTooltip.types';
import { ChartsAxisHighlight, ChartsAxisHighlightProps } from '../ChartsAxisHighlight';
Expand Down Expand Up @@ -187,7 +187,7 @@ const SparkLineChart = React.forwardRef(function SparkLineChart(
]}
yAxis={[
{
id: DEFAULT_X_AXIS_KEY,
id: DEFAULT_Y_AXIS_KEY,
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like we were using X_AXIS_ID on yAxis for sparkline 🫠

...yAxis,
},
]}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('useSkipAnimation', () => {
const errorMessage2 =
'It looks like you rendered your component outside of a ChartsContainer parent component.';
const errorMessage3 = 'The above error occurred in the <UseSkipAnimation> component:';
const expextedError =
const expectedError =
reactMajor < 19
? [errorMessage1, errorMessage2, errorMessage3]
: `${errorMessage1}\n${errorMessage2}`;
Expand All @@ -49,7 +49,7 @@ describe('useSkipAnimation', () => {
<UseSkipAnimation />
</ErrorBoundary>,
),
).toErrorDev(expextedError);
).toErrorDev(expectedError);

expect((errorRef.current as any).errors).to.have.length(1);
expect((errorRef.current as any).errors[0].toString()).to.include(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe('useHighlighted', () => {
const errorMessage2 =
'It looks like you rendered your component outside of a ChartsContainer parent component.';
const errorMessage3 = 'The above error occurred in the <UseHighlighted> component:';
const expextedError =
const expectedError =
reactMajor < 19
? [errorMessage1, errorMessage2, errorMessage3]
: `${errorMessage1}\n${errorMessage2}`;
Expand All @@ -34,7 +34,7 @@ describe('useHighlighted', () => {
<UseHighlighted />
</ErrorBoundary>,
),
).toErrorDev(expextedError);
).toErrorDev(expectedError);

expect((errorRef.current as any).errors).to.have.length(1);
expect((errorRef.current as any).errors[0].toString()).to.include(
Expand Down
1 change: 1 addition & 0 deletions packages/x-charts/src/hooks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ export {
useScatterSeries as unstable_useScatterSeries,
} from './useSeries';
export * from './useLegend';
export { useChartGradientId, useChartGradientIdObjectBound } from './useChartGradientId';
41 changes: 41 additions & 0 deletions packages/x-charts/src/hooks/useChartGradientId.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import * as React from 'react';
import { expect } from 'chai';
import { createRenderer, screen } from '@mui/internal-test-utils';
import { useChartGradientId, useChartGradientIdObjectBound } from './useChartGradientId';
import { ChartDataProvider } from '../context';

function UseGradientId() {
const id = useChartGradientId('test-id');
return <div>{id}</div>;
}

function UseGradientIdObjectBound() {
const id = useChartGradientIdObjectBound('test-id');
return <div>{id}</div>;
}

describe('useChartGradientId', () => {
const { render } = createRenderer();

it('should properly generate a correct id', () => {
render(
<ChartDataProvider series={[]} height={100} width={100}>
<UseGradientId />
</ChartDataProvider>,
);

expect(screen.getByText(/:\w+:-gradient-test-id/)).toBeVisible();
});

describe('useChartGradientIdObjectBound', () => {
it('should properly generate a correct id', () => {
render(
<ChartDataProvider series={[]} height={100} width={100}>
<UseGradientIdObjectBound />
</ChartDataProvider>,
);

expect(screen.getByText(/:\w+:-gradient-test-id-object-bound/)).toBeVisible();
});
});
});
51 changes: 51 additions & 0 deletions packages/x-charts/src/hooks/useChartGradientId.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use client';
import * as React from 'react';
import { useChartId } from './useChartId';
import { AxisId } from '../models/axis';

/**
* Returns a function that generates a gradient id for the given axis id.
*/
export function useChartGradientIdBuilder() {
const chartId = useChartId();
return React.useCallback((axisId: AxisId) => `${chartId}-gradient-${axisId}`, [chartId]);
}

/**
* Returns a function that generates a gradient id for the given axis id.
*/
export function useChartGradientIdObjectBoundBuilder() {
const chartId = useChartId();
return React.useCallback(
(axisId: AxisId) => `${chartId}-gradient-${axisId}-object-bound`,
[chartId],
);
}

/**
* Returns a gradient id for the given axis id.
*
* Can be useful when reusing the same gradient on custom components.
*
* For a gradient that respects the coordinates of the object on which it is applied, use `useChartGradientIdObjectBound` instead.
*
* @param axisId the axis id
* @returns the gradient id
*/
export function useChartGradientId(axisId: AxisId) {
return useChartGradientIdBuilder()(axisId);
}

/**
* Returns a gradient id for the given axis id.
*
* Can be useful when reusing the same gradient on custom components.
*
* This gradient differs from `useChartGradientId` in that it respects the coordinates of the object on which it is applied.
*
* @param axisId the axis id
* @returns the gradient id
*/
export function useChartGradientIdObjectBound(axisId: AxisId) {
return useChartGradientIdObjectBoundBuilder()(axisId);
}
4 changes: 2 additions & 2 deletions packages/x-charts/src/hooks/useSeries.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('useSeries', () => {
const errorMessage2 =
'It looks like you rendered your component outside of a ChartDataProvider.';
const errorMessage3 = 'The above error occurred in the <UseSeries> component:';
const expextedError =
const expectedError =
reactMajor < 19
? [errorMessage1, errorMessage2, errorMessage3]
: [errorMessage1, errorMessage2].join('\n');
Expand All @@ -33,7 +33,7 @@ describe('useSeries', () => {
<UseSeries />
</ErrorBoundary>,
),
).toErrorDev(expextedError);
).toErrorDev(expectedError);

expect((errorRef.current as any).errors).to.have.length(1);
expect((errorRef.current as any).errors[0].toString()).to.include(errorMessage1);
Expand Down
4 changes: 2 additions & 2 deletions packages/x-charts/src/hooks/useSvgRef.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ describe('useSvgRef', () => {
'It looks like you rendered your component outside of a ChartDataProvider.',
'The above error occurred in the <UseSvgRef> component',
];
const expextedError = reactMajor < 19 ? errorMessages : errorMessages.slice(0, 2).join('\n');
const expectedError = reactMajor < 19 ? errorMessages : errorMessages.slice(0, 2).join('\n');

expect(() =>
render(
<ErrorBoundary ref={errorRef}>
<UseSvgRef />
</ErrorBoundary>,
),
).toErrorDev(expextedError);
).toErrorDev(expectedError);

expect((errorRef.current as any).errors).to.have.length(1);
expect((errorRef.current as any).errors[0].toString()).to.include(
Expand Down
Loading
Loading