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] Feedback/issues found while migrating the Toolpad chart component to x-charts #10140

Closed
2 tasks done
apedroferreira opened this issue Aug 25, 2023 · 10 comments
Closed
2 tasks done
Labels
bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module!

Comments

@apedroferreira
Copy link
Member

apedroferreira commented Aug 25, 2023

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

PR in Toolpad, just for context: mui/toolpad#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. -> [charts] Add grid layout #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 [charts] Improve the management of the text (legend only) #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',? -> [charts] Improve the management of the text (legend only) #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

@apedroferreira apedroferreira added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 25, 2023
@apedroferreira apedroferreira changed the title Feedback/issues found while migrating the Toolpad chart component to x-charts [charts] Feedback/issues found while migrating the Toolpad chart component to x-charts Aug 25, 2023
@apedroferreira apedroferreira added the component: charts This is the name of the generic UI component, not the React module! label Aug 25, 2023
@alexfauquette
Copy link
Member

alexfauquette commented Aug 25, 2023

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

  • Document the responsive behavior
  • [ ]

@apedroferreira
Copy link
Member Author

apedroferreira commented Aug 25, 2023

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

This comment was marked as outdated.

@flaviendelangle flaviendelangle removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 1, 2023
@apedroferreira
Copy link
Member Author

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
Copy link
Contributor

wascou commented Sep 15, 2023

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

@apedroferreira
Copy link
Member Author

apedroferreira commented Sep 18, 2023

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
Copy link
Member

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

@apedroferreira
Copy link
Member Author

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
Copy link
Member

With #10648

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

image

@apedroferreira
Copy link
Member Author

apedroferreira commented Oct 18, 2023

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

@DanailH DanailH added the bug 🐛 Something doesn't work label Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

5 participants