[chart] Make smarter default domain limit #18506
Conversation
|
Deploy preview: https://deploy-preview-18506--material-ui-x.netlify.app/ Updated pages: Bundle size reportTotal Size Change: 🔺+19.9KB(+0.15%) - Total Gzip Change: 🔺+6.02KB(+0.15%) Show details for 100 more bundles (22 more not shown)@mui/x-charts parsed: 🔺+798B(+0.26%) gzip: 🔺+256B(+0.28%) |
Why can't we keep it off and the user can set We can then just flip it next release |
|
Or is this a proposal for all future breaking changes? 🤔 |
Yes, my opinion is that a next major in march is too far for those modification. In the data grid we had a prop But for chart we should move that at a hight level.
We can, but I'm not in love with the idea of adding this strict to all the x-axis on lin chart demos. XHihc is necessary because some of them looks weird on mobile phone (due to the impact of a small with for the same number of ticks) |
|
Got it, in that case either a plugin/ChartsDataProvider approach or a overarching context can work for us. |
Can't we follow the same approach? All charts and the |
CodSpeed Performance ReportMerging #18506 will not alter performanceComparing Summary
|
docs/pages/_app.js
Outdated
| @@ -162,6 +163,16 @@ function loadDependencies() { | |||
| ); | |||
| } | |||
|
|
|||
| const experimentaChartFeatures = createTheme({ | |||
There was a problem hiding this comment.
| const experimentaChartFeatures = createTheme({ | |
| const experimentalChartFeatures = createTheme({ |
docs/pages/_app.js
Outdated
| @@ -331,7 +342,7 @@ function AppWrapper(props) { | |||
| <PageContext.Provider value={pageContextValue}> | |||
| <ThemeWrapper> | |||
| <DocsStyledEngineProvider cacheLtr={emotionCache}> | |||
| {children} | |||
| <ThemeProvider theme={experimentaChartFeatures}>{children}</ThemeProvider> | |||
There was a problem hiding this comment.
I'm wondering if this will reduce the reproducibility of our docs.
When someone creates a sandbox from our docs, these experimental features won't be enabled, will they? So the sandboxes will look different from the docs.
Furthermore, users copying code from our docs will see different results in their environment because they don't know they need to enable this experimental feature.
I know it's verbose, but maybe we should use the strict domain limit in our demos and flip the default in v9?
We could also keep these experimental chart features to ease migration, but I'm not sure we should enable them in our docs.
There was a problem hiding this comment.
I will do the copy paste to most of our line demos in a dedicated PR to avoid adding more diff in this one
| { date: new Date(1990, 0, 1), fr: 28129, gb: 26189, dl: 25391 }, | ||
| { date: new Date(1991, 0, 1), fr: 28294.264, gb: 25792.014, dl: 26769.96 }, |
There was a problem hiding this comment.
Why remove these data points?
There was a problem hiding this comment.
To make the demo more explicit. 1990 is already a rounded number.
I also considered removing it only for the domain limit demo. But demo could also start in 1992
There was a problem hiding this comment.
I also considered removing it only for the domain limit demo
Yeah, I think that makes more sense
...ternals/plugins/corePlugins/useChartExperimentalFeature/useChartExperimentalFeature.types.ts
Outdated
Show resolved
Hide resolved
|
|
||
| export interface ChartExperimentalFeatures { | ||
| /** | ||
| * Default domainLimit to strict for line chart x-axis. |
There was a problem hiding this comment.
I find it a bit weird that we can enable this for a ScatterChart, for example, even though it might not have any effect.
It's probably fine, though
There was a problem hiding this comment.
Yeah the root cause is the fact it in a common experimentalFeature object. We could remove it from all other charts for now.
And find a better naming than strictDomainLimit tho highlight the facts it's related to line plotting
There was a problem hiding this comment.
preferStrictDomainInLineCharts?
There was a problem hiding this comment.
A bit long but sounds better 👍
| @@ -55,6 +56,7 @@ type ComputeCommonParams<T extends ChartSeriesType = ChartSeriesType> = { | |||
| zoomMap?: Map<AxisId, ZoomData>; | |||
| zoomOptions?: Record<AxisId, DefaultizedZoomOptions>; | |||
| getFilters?: GetZoomAxisFilters; | |||
| experimental_strictDomainLimit?: boolean; | |||
There was a problem hiding this comment.
computeAxisValue doesn't need to know that this is an experimental flag.
We can just call it something like this:
| experimental_strictDomainLimit?: boolean; | |
| defaultLineSeriesToStrictDomain?: boolean; |
or
| experimental_strictDomainLimit?: boolean; | |
| preferStrictDomainLimit?: boolean; |
There was a problem hiding this comment.
I went with experimental because it will be useless in next major. So it's a way to tell "Hey you can remove this param"
There was a problem hiding this comment.
We can add a comment for that, but not that big of an issue
...es/x-charts/src/internals/plugins/featurePlugins/useChartCartesianAxis/getAxisDomainLimit.ts
Outdated
Show resolved
Hide resolved
...es/x-charts/src/internals/plugins/featurePlugins/useChartCartesianAxis/getAxisDomainLimit.ts
Show resolved
Hide resolved
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
| import { dataset } from './GDPperCapita'; | ||
|
|
||
| export default function LineDefaultDomainLimit() { | ||
| const [preferStrictDomainInLineCharts, setpreferStrictDomainInLineCharts] = |
There was a problem hiding this comment.
💅
| const [preferStrictDomainInLineCharts, setpreferStrictDomainInLineCharts] = | |
| const [preferStrictDomainInLineCharts, setPreferStrictDomainInLineCharts] = |
The line charts with a x-axis
domainLimit: 'nice'is a bit weird because it ads extra with space on left/right if the first/last data point are not nice ones.This PR moves the default of x-axis for line chart to 'strict'.
This can either looks like a breaking change
I would like to make it opt-in before v9, but not sure how to do it. I'm thinking about either
ChartsBreakingChangeto put at the root of the projectexperimentalDefaultDomainLimitset attruefor the ChartDataProvider