mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.16k stars 1.3k forks source link

[charts] Feedback/issues found while migrating the Toolpad chart component to x-charts #10140

Closed apedroferreira closed 10 months ago

apedroferreira commented 1 year ago

Duplicates

Latest version

Steps to reproduce 🕹

PR in Toolpad, just for context: https://github.com/mui/mui-toolpad/pull/2500

Current behavior 😯

Most of the implementation went great, just found a few issues along the way and some things that I wasn't sure how to do. Many of them might be just me missing something or lacking context, so I'm really just listing everything I thought could be worth mentioning to provide as much feedback as possible.

Expected behavior 🤔

Features

  1. How can I make the chart full-width or even full-height? Both would be useful for Toolpad, at least full-width would be. The current height and width props seem to only take numbers. solved
  2. I was using a combined chart with the <ChartContainer /> component, and noticed that in the line charts there are no dots along the lines as there are in https://mui.com/x/react-charts/lines/. Is this intentional? Not very important and might make sense for them not to show in some cases, but just making sure. solved
  3. Recharts (and Hackathon Charts 😉) allows for showing a grid behind the chart that matches the axis coordinates. Does x-charts allow for that somehow? Also not very important, just a possible nice-to-have. -> #10158

Errors

I ran into a few errors sometimes that didn't really help me towards figuring out what was wrong:

  1. I have a <ChartsTooltip /> component inside the ChartContainer, but if no chart is plotted, because for example the axis properties do not match the series data being passed, hovering the chart shows the error Cannot read properties of undefined (reading 'toLocaleString'). I expected the Tooltip not to show any errors in this scenario even though no chart is being shown. Update: this is because I was using linear scaleType for a line chart, and the x-axis had string values. Using point fixed it, so this isn't an issue anymore. I can try to provide a repro if you decide to look into this though.
  2. For example, if we forget to pass the yAxis prop to ChartContainer, we get an error that says it cannot get .scale of undefined. This specific error seems to show in many similar cases where some chart configuration is wrong, maybe the error messages could be more useful somehow? This yAxis prop is also marked as optional in the types, so maybe I did not expect it to cause an error.

Minor issues

  1. The default spacing in the chart legend seems to get them cut-off quite easily, I had to customize --ChartsLegend-rootSpacing to a bigger value (25px). Not sure it could/should be more responsive either instead of just manual spacing? -> Shuld be solved by #10138

Docs

  1. The example here (https://mui.com/x/react-charts/legend/) at the end says '--ChartsLegend-itemMarkSize': 20px,, for example, I guess it should be '--ChartsLegend-itemMarkSize': '20px',? -> #10138 may lead to a new interface without CSS vars

Context 🔦

None of the remaining issues are blocking, so nothing too important anymore!

Your environment 🌎

Google Chrome

System:
    OS: macOS 13.4.1
  Binaries:
    Node: 20.3.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.6.7 - /usr/local/bin/npm
  Browsers:
    Chrome: 116.0.5845.110
    Edge: Not Found
    Safari: 16.5.2
  npmPackages:
    @emotion/react:  11.11.1
    @emotion/styled:  11.11.0
    @mui/base:  5.0.0-beta.11
    @mui/core-downloads-tracker:  5.14.5
    @mui/icons-material:  5.14.3
    @mui/lab:  5.0.0-alpha.140
    @mui/material:  5.14.5
    @mui/monorepo: https://github.com/mui/material-ui.git => 5.14.4
    @mui/private-theming:  5.14.5
    @mui/styled-engine:  5.13.2
    @mui/system:  5.14.5
    @mui/toolpad:  0.1.23
    @mui/toolpad-components:  0.1.23
    @mui/toolpad-core:  0.1.23
    @mui/toolpad-utils:  0.1.23
    @mui/types:  7.2.4
    @mui/utils:  5.14.5
    @mui/x-charts: https://pkg.csb.dev/mui/mui-x/commit/290ecadb/@mui/x-charts => 6.0.0-alpha.7
    @mui/x-data-grid:  6.11.1
    @mui/x-data-grid-pro:  6.11.1
    @mui/x-date-pickers:  6.11.1
    @mui/x-date-pickers-pro:  6.11.1
    @mui/x-license-pro:  6.10.2
    @types/react:  18.2.20
    react:  18.2.0
    react-dom:  18.2.0
    typescript: 5.1.6 => 5.1.6

Order ID or Support key 💳 (optional)

No response

alexfauquette commented 1 year ago

Quick answers

For full-width/full-height you can use <ResponsiveChartContainer /> and do not provide a width or a height. It will scale to fit the parent size. This should be documented. Not sure where to put this piece of information.

About missing dots in composed line charts, it's because line charts have 3 sub-components: <AreaPlot/>, <LinePlot/>, and <MarkPlot/>. The one you're looking for (adding dots) is MarkPlot (e.g. line chart source code)

TODO

apedroferreira commented 1 year ago

For full-width/full-height you can use <ResponsiveChartContainer /> and do not provide a width or a height. It will scale to fit the parent size. This should be documented. Not sure where to put this piece of information.

About missing dots in composed line charts, it's because line charts have 3 sub-components: <AreaPlot/>, <LinePlot/>, and <MarkPlot/>. The one you're looking for (adding dots) is MarkPlot (e.g. line chart source code)

Thanks, those solved it! Maybe you can mention the responsive container here after mentioning the ChartContainer? https://mui.com/x/react-charts/ Just a possiblity.

apedroferreira commented 1 year ago

Another possible issue, adding here not to spam too much (hopefully):

With Recharts we used to have these line charts to show number of downloads in https://github.com/mui/mui-public/tree/master/tools-public:

Screenshot 2023-08-31 at 17 32 04

But I need to use a band scale type for the line plot to show strings in the X axis, and I can't customize the number of ticks in the X-axis, which means it gets pretty crowded:

Screenshot 2023-08-31 at 18 03 11

The ticks do not align either, not sure if we should do anything about that or if I should try using another type of scale.

Nevermind, the point scale works after all.

apedroferreira commented 1 year ago

Another issue: Is there any way that I can customize the number of ticks shown, with the same data? I'm using a scaleType of point here. tickNumber doesn't seem to do anything.

Screenshot 2023-09-01 at 18 44 33
wascou commented 1 year ago

hey @apedroferreira ticknumber works only if you have a time serie, meaning your xAxis data is an array of "Date" objects

apedroferreira commented 1 year ago

hey @apedroferreira ticknumber works only if you have a time serie, meaning your xAxis data is an array of "Date" objects

I see, thanks for pointing that out! recharts automatically picked only some ticks to show even if the xAxis data were strings, not sure if x-charts should do the same. From the charts we were using in our internal Toolpad applications I guess it should be useful.

alexfauquette commented 1 year ago

Effectively, band and point are items for which ticknumber has no impact. In your case, I

There are two aspects: ticks and ticks label

apedroferreira commented 1 year ago

Effectively, band and point are items for which ticknumber has no impact. In your case, I

There are two aspects: ticks and ticks label

  • tick label: should be fixed with the text size management. When we know how much space a tick label needs, we could display tick labels such that they do not overlap
  • tick: In your example, it's not a problem. With more data points, we could whish to provide a way to display only a subset of ticks

Ok, thanks for the answer! So I guess the text fixes should solve the issue. :)

alexfauquette commented 1 year ago

With #10648

The axis ticks should be solved since label overflow is automatically avoided

image

apedroferreira commented 1 year ago

With #10648

The axis ticks should be solved since label overflow is automatically avoided

image

I just tried with the latest version in the PR and it looks perfect!! Thanks!

Screenshot 2023-10-18 at 19 34 13