-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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] Use plugins to define series extremum and colors #13397
Conversation
Deploy preview: https://deploy-preview-13397--material-ui-x.netlify.app/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice, added a suggestion based on my finding on the other pr.
export function useColorProcessor<T extends ChartSeriesType>( | ||
seriesType: T, | ||
): ColorProcessorsConfig<ChartSeriesType>[T]; | ||
export function useColorProcessor(): ColorProcessorsConfig<ChartSeriesType>; | ||
export function useColorProcessor(seriesType?: ChartSeriesType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following #13396
We could allow ColorProcessor
to trim down the correct types itself
// plugin.ts
export type ColorProcessor<T extends ChartSeriesType> = T extends ChartSeriesType
? (
series: DefaultizedSeriesType<T>,
xAxis?: AxisDefaultized,
yAxis?: AxisDefaultized,
zAxis?: ZAxisDefaultized,
) => (dataIndex: number) => string
: never;
And then on this file
export function useColorProcessor<T extends ChartSeriesType>( | |
seriesType: T, | |
): ColorProcessorsConfig<ChartSeriesType>[T]; | |
export function useColorProcessor(): ColorProcessorsConfig<ChartSeriesType>; | |
export function useColorProcessor(seriesType?: ChartSeriesType) { | |
export function useColorProcessor<T extends ChartSeriesType>(seriesType: T): ColorProcessor<T>; | |
export function useColorProcessor(): ColorProcessorsConfig<ChartSeriesType>; | |
export function useColorProcessor(seriesType?: ChartSeriesType) { |
// Components | ||
export * from './components/ChartsAxesGradients'; | ||
|
||
// hooks | ||
export { useReducedMotion } from '../hooks/useReducedMotion'; | ||
export { useSeries } from '../hooks/useSeries'; | ||
|
||
// utils | ||
export * from './defaultizeValueFormatter'; | ||
export * from './configInit'; | ||
|
||
// contexts | ||
|
||
export * from '../context/CartesianContextProvider'; | ||
export * from '../context/DrawingProvider'; | ||
export * from '../context/InteractionProvider'; | ||
export * from '../context/SeriesContextProvider'; | ||
export * from '../context/ZAxisContextProvider'; | ||
|
||
// series configuration | ||
export * from '../models/seriesType/config'; | ||
export * from '../models/seriesType/common'; | ||
|
||
export * from '../models/helpers'; | ||
export * from '../models/z-axis'; | ||
export * from '../models/axis'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file will probably cause a lot of unnecessary cyclic deps 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think so?
The internal/index.ts
is exporting all the elements from the community package we need for creating the pro one but don't want to expose to the users.
- they are not documented in API pages
- they can only be imported with
from '@mui/x-charts/internal
which is assumed to be enough to know those API are to be considered unstable
So the community package has 3 kinds of elements:
- Those publically exported by
@mui/x-charts
and@mui/x-charts/Xxxx
(in blue) - Those privately exported by
@mui/x-charts/internals
(in red) - Those not exported (in yellow)
Since import/export is only going from MIT to pro, this should not create any loop. But I might miss something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internals itself is a folder with content, and it is exporting files and folders outside of itself, on the same level.
Eg: export * from '../context/CartesianContextProvider';
The ../context
is at the same level as ../internals
If on the CartesianContextProvider.tsx
file, we remove import { SeriesId } from '../models/seriesType/common';
and reimport it, it will prefer the to import from ../internals
and will cause a cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By convention, we never import from /internals
. You can import from internals/...
but not the index file. This index file is only here to be used by the pro packages. THis convention should prevent that kind of issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would be useful to have an eslint rule about it then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never got any issues with that in 3 years 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 my auto import creates cyclic deps instantly with these changes, then I have to manually change the import. But we can look at that in another MR
The extremum commit is straightforward. Whereas, the color one required to add a context.
We currently use a function that takes the series and the different axes config and outputs a getter
dataINdex => color
.This function is used at multiple places. So I'm introducing a provider to still be able to share it with all items