-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[charts] Introduce the radar chart #16406
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
Conversation
Deploy preview: https://deploy-preview-16406--material-ui-x.netlify.app/ Updated pages: |
37ed9fc
to
a95fddc
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
a589b3a
to
e1b7842
Compare
CodSpeed Performance ReportMerging #16406 will not alter performanceComparing Summary
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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.
Good job! Left some comments because I don't get a lot of stuff 😛
seriesProp: RadarSeriesType; | ||
itemIdentifier: RadarItemIdentifier; | ||
valueType: number; | ||
polar: true; |
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.
More of a nitpick, and we don't need to solve it now since we're just repeating the established pattern of using cartesian: true
, but would it make sense to have a type: 'cartesian'
or type: 'polar'
instead?
params: UseChartPolarAxisParameters; | ||
defaultizedParams: UseChartPolarAxisDefaultizedParameters; | ||
state: UseChartPolarAxisState; | ||
// instance: UseChartPolarAxisInstance; |
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.
Bump to ensure this comment isn't forgotten 😄
margin={{ top: 20 }} | ||
series={[ | ||
{ | ||
// label: 'Lisa', |
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.
// label: 'Lisa', |
|
||
export default function CompositionExample() { | ||
return ( | ||
<RadarDataProvider |
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 wonder if it makes sense to have this RadarDataProvider
.
From my understanding, we already have three tiers of decreasing abstraction: LineChart
, ChartContainer
and ChartDataProvider
. With RadarDataProvider
, it seems we'll now have 4 tiers: RadarChart
, ChartContainer
, RadarDataProvider
and ChartDataProvider
.
Does it make sense to have the distinction between RadarDataProvider
and ChartDataProvider
?
I'm questioning because either we'll have to add docs on how to use ChartDataProvider
with a radar chart, or we'll probably need to update the generic composition page to mention that some of that information doesn't apply to radar charts.
What are we trying to accomplish with RadarDataProvider
that can't be done with ChartDataProvider
?
My point of view might not make sense since I don't have much experience with this, but I'm a bit confused so I assumed that users might also face the same problem. Curious to hear your opinion!
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.
Yeah I have some concerns as well with the DataProvider
, currently you can't use zoom
if you use composition. And if we add a different data provider for each of the charts it might get messy 😓
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.
What are we trying to accomplish with RadarDataProvider that can't be done with ChartDataProvider?
The mapping from the radar
props to polar axes.
The idea is to provide a simpler API such that users can not shoot in their feet, and allowing us to make assumption about the links between the axes.
Technically they could use the DataProvider if they respect the relation between the radial axes, and the rotation one.
I have some concerns as well with the DataProvider, currently you can't use zoom if you use composition.
That's an issue about plugins propagation. The DataProviderProps
should defaultize its plugins with the default MIT plugins plus the zoom one
For me the strategy is the following
- A general composition is ChartContainer[Pro] = ChartDataProvider[Pro] + ChartSurface[Pro]
- Custom Data provider for charts with dedicated API
The idea being that the ChartDataProvider
is a technical description of the charts, and the custom data provider is a simplification because not all the combinations of axes and data make sense. For example, the funnel charts does not really need flexible axes. They have some combination that are useful and could be extracted in a simplified API.
The messy aspect comes from the fact that we currently have only Cartesian charts that can be mixed together + the pie chart that is poorly documented. The composition docs could effectively be more explicit about the two approach about composition.
axisIndex: number, | ||
formattedSeries: ProcessedSeries<TSeriesType>, | ||
) => { | ||
const charTypes = Object.keys(seriesConfig).filter(isPolarSeriesType); |
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.
Nit: did you mean chartTypes
I guess something like polarSeries
would maybe make more sense?
const charTypes = Object.keys(seriesConfig).filter(isPolarSeriesType); | |
const polarSeries = Object.keys(seriesConfig).filter(isPolarSeriesType); |
* The number of divisions in the radar grid. | ||
* @default 5 | ||
*/ | ||
divisionNumber?: number; |
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.
Nit: would it make sense to call this divisions
or slices
?
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.
Slices would be the V
shapes. Like a slice of pizza. I found divisionNumber
a bit confusing too. divisions
or internalDivisions
, though the first is probably better.
*/ | ||
name: string; | ||
/** | ||
* The minimal value of the domain. |
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 we mean "minimum"?
* The minimal value of the domain. | |
* The minimum value of the domain. |
metrics: string[] | MetricConfig[]; | ||
/** | ||
* The default max value for axis. | ||
* If will be override is `metrics` contains a `max` property. |
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.
* If will be override is `metrics` contains a `max` property. | |
* It will be overridden if `metrics` contains a `max` property. |
|
||
export interface RadarConfig { | ||
/** | ||
* The different metrics shown by radar. |
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 is more of a nit, but I'm not sure "different" is adding much here.
* The different metrics shown by radar. | |
* The metrics shown by radar. |
*/ | ||
max?: number; | ||
/** | ||
* The angle of the first axis (in deg) |
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.
Would it make sense to say something like "The starting angle of the rotation axis (in degrees)"? It might not be clear that the first axis is the rotation axis, or is it?
Should we fix darkmode in this PR? |
/** | ||
* The number of steps in the radar grid. | ||
*/ | ||
divisionNumber?: number; |
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 prop does nothing. I guess it was moved into the root props but forgotten here?
/** | ||
* If `true` show marks at value position. | ||
*/ | ||
showMark?: boolean; |
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.
showMark
and valueFormatter
doesn't seem to be working.
import ChartsUsageDemo from 'docsx/src/modules/components/ChartsUsageDemo'; | ||
import { Unstable_RadarChart as RadarChart } from '@mui/x-charts/RadarChart'; | ||
|
||
export default function DemoRadarNoSnap() { |
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.
We can remove the NoSnap
from this file. This demo seems useful to snap 👍
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
@@ -6,7 +6,7 @@ import { useItemHighlightedGetter } from '../../hooks/useItemHighlightedGetter'; | |||
import { SeriesId } from '../../models/seriesType/common'; | |||
|
|||
/** | |||
* | |||
* THis hook provides all the data needed to display radar series. |
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 hook provides all the data needed to display radar series. | |
* This hook provides all the data needed to display radar series. |
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.
Code looks good! Minor comments regarding the docs
*/ | ||
min?: number; | ||
/** | ||
* The maximal value of the domain. |
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.
Did you mean maximum?
* The maximal value of the domain. | |
* The maximum value of the domain. |
metrics: string[] | MetricConfig[]; | ||
/** | ||
* The default max value for radius axes. | ||
* It will be override if `metrics` contains a `max` property. |
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.
* It will be override if `metrics` contains a `max` property. | |
* It will be overridden if `metrics` contains a `max` property. |
/** | ||
* The metrics shown by radar. | ||
*/ | ||
metrics: string[] | MetricConfig[]; |
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.
From the code, it seems we actually support Array<string | MetricConfig>
because we're handling each metric separately:
radar.metrics.map((m) => {
const { name, min = 0, max = radar.max } = typeof m === 'string' ? { name: m } : m;
return {
id: name,
label: name,
scaleType: 'linear' as const,
min,
max,
};
Not sure if there's any benefit in allowing that, but just wondering if we should use Array<string | MetricConfig>
here.
}); | ||
}); | ||
|
||
// eslint-disable-next-line mocha/max-top-level-suites |
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 wonder if we should remove this rule altogether? Do you know why we have it enabled?
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.
It will be removed once we stop depending on mocha. Theoretically mocha doesn't support multiple describes. Technically it doesn't have a problem with that...
Right now it is part of the defaults, so I just opted to leave it be and change it later when we are in position to do so globally
* The rotation-axes IDs sorted by order they got provided. | ||
*/ | ||
rotationAxisIds: AxisId[]; | ||
/** | ||
* The radius-axes IDs sorted by order they got provided. |
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.
* The rotation-axes IDs sorted by order they got provided. | |
*/ | |
rotationAxisIds: AxisId[]; | |
/** | |
* The radius-axes IDs sorted by order they got provided. | |
* The rotation-axes IDs sorted by order they were provided. | |
*/ | |
rotationAxisIds: AxisId[]; | |
/** | |
* The radius-axes IDs sorted by order they were provided. |
|
||
## Basics | ||
|
||
A radar chart is defined by two main props. |
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.
A radar chart is defined by two main props. | |
A radar chart is defined by two main props: |
|
||
The radar chart displays a grid behind the series that can be configured with | ||
|
||
- `startAngle` The rotation angle of the entire chart in degree |
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.
- `startAngle` The rotation angle of the entire chart in degree | |
- `startAngle` The rotation angle of the entire chart in degrees |
The radar chart displays a grid behind the series that can be configured with | ||
|
||
- `startAngle` The rotation angle of the entire chart in degree | ||
- `divisions` The number of division of the grid |
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.
- `divisions` The number of division of the grid | |
- `divisions` The number of divisions of the grid |
In this example, we uses `RadarSeriesArea` and `RadarSeriesMarks` to modify the order of the elements: | ||
all the marks are on top of all the path. | ||
And we apply different properties based on the series id. |
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.
In this example, we uses `RadarSeriesArea` and `RadarSeriesMarks` to modify the order of the elements: | |
all the marks are on top of all the path. | |
And we apply different properties based on the series id. | |
In this example, we use `RadarSeriesArea` and `RadarSeriesMarks` to modify the order of the elements so that all the marks render above the paths. | |
Additionally, we apply different properties based on the series ID. |
* The id of the series To display. | ||
* If undefined all series are display. |
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.
* The id of the series To display. | |
* If undefined all series are display. | |
* The id of the series to display. | |
* If undefined all series are displayed. |
This PR is a rough first version of the Radar chart #7925. Lots of features are missing.
The idea is to validate the data structure, and it's mapping to a rotation/radius scale system. If ok, we can merge this one and add features in distinct PR such that we keep them small enough to be relevant (I don't expect someone to read 200 files with 10 different features and spot any bug or room for improvement)
Main idea
The polar plugin introduce the support of rotatio/radius axes that work similarly to the x/y axes
The radar don't expose directly those axies. It uses an intermediate
metrics
to make sure the radar has only one rotation axis with scale point, and one radius axis per metric.Then we have one component per item rendered (grid, area, and marks)
Features
Future features
Preview: https://deploy-preview-16406--material-ui-x.netlify.app/x/react-charts/radar/
Changelog