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] Add Gauge component #11996

Merged
merged 22 commits into from Feb 15, 2024
Merged

[charts] Add Gauge component #11996

merged 22 commits into from Feb 15, 2024

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Feb 8, 2024

Fix #2903

Tasks

  • Display arc elements
  • Manage a11y
  • Make it customizable (still to document CSS behavior)
  • Support animation? -> maybe wait for feedback
  • Display value
    • Where?
    • How? (can be SVG Text or a simple <p> displayed bellow or on top)
  • Document how to use the gauge with other component to encourage using the component with other standard HTML

Preview https://deploy-preview-11996--material-ui-x.netlify.app/x/react-charts/gauge/

@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Feb 8, 2024
@mui-bot
Copy link

mui-bot commented Feb 8, 2024

Deploy preview: https://deploy-preview-11996--material-ui-x.netlify.app/

Updated pages:

Generated by 🚫 dangerJS against 8dfeffd

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented Feb 9, 2024

Just a quick comment, I believe the gauge charts component should be able to display a pointer/needle as well and not rely exclusively on the "progress bar" behavior (in fact, the use of pointers feels like should be the default behavior).
A search for "gauge" shows how dominant the visualization with "pointer" is.
image

@alexfauquette
Copy link
Member Author

alexfauquette commented Feb 9, 2024

The issue is that the single component can not do all at once.

I made it such that it should be easy to add/remove components. From the issue benchmark, other libraries are at 50/50 between having a pointers or not.
https://www.notion.so/mui-org/Gauge-benchmark-1ca05e2b69b34be1ab1bdc5f2076d0f9

The idea of this default gauge come from this issue (by the way @johnsonav1992 you might be interested in this new component) Assuming that if a user created it might interest others. I'm going to ask in the issue
#11309

After there is the technical aspect. The pointer only make sens if we create a reference to point. So either displaying ticks, or arc colors. That's additional complex components I would prefer to share with other charts (for example supporting varying colors according to the value could also benefit to line and bar chart) to have it working well on all it's a non negligeable API thinking

@johnsonav1992
Copy link

johnsonav1992 commented Feb 11, 2024

@alexfauquette This is great!! I was hoping for a default "donut" chart option like what you are building here (you are calling it a gauge but same thing) when I was building the donut for our app at my work, but I was able to do it with the pie charts after some tinkering around to get everything to look right (getting the color of the back arc of the donut to be different than the front was probably the hardest part). However, this will definitely reduce the overhead that people will need to deal with in terms of using the documentation to create such a donut chart. Appreciate all your hard work and innovation in this library! We are loving using it so far!

@alexfauquette alexfauquette marked this pull request as ready for review February 12, 2024 13:50
Comment on lines 41 to 46
// {
// title: 'Gauge',
// srcLight: '/static/x/component-illustrations/gauge-light.png',
// srcDark: '/static/x/component-illustrations/gauge-dark.png',
// href: '/x/react-charts/gauge/',
// },
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove comments when the image is available

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added illustrations for that in a separate PR → #12041

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick copywriting improvements here and there!

docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
Comment on lines +82 to +84
- GaugeReferenceArc
- GaugeValueArc
- GaugeValueText
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- GaugeReferenceArc
- GaugeValueArc
- GaugeValueText
- `<GaugeReferenceArc />`
- `<GaugeValueArc />`
- `<GaugeValueText />`

docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! 👏
Very nice improvement to the charts portfolio. 👍

WDYT about adding this component to themeAugmentation?
Do we want it?
Based on the styled components it would seem that we expect the theme to be overridden, but it's not doable at the moment. 🤔

docs/data/charts/gauge/gauge.md Show resolved Hide resolved
docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
docs/data/charts/gauge/gauge.md Outdated Show resolved Hide resolved
packages/x-charts/src/Gauge/GaugeProvider.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/Gauge/GaugeProvider.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/Gauge/GaugeProvider.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/Gauge/utils.ts Outdated Show resolved Hide resolved
packages/x-charts/src/Gauge/Gauge.tsx Outdated Show resolved Hide resolved
@TheOneTheOnlyJJ
Copy link

TheOneTheOnlyJJ commented Feb 14, 2024

The Arcs configuration demo from the docs breaks if either the innerRadius or the outerRadius values are empty (deleted): https://deploy-preview-11996--material-ui-x.netlify.app/x/react-charts/gauge/#arcs-configuration

Co-authored-by: Danilo Leal <67129314+danilo-leal@users.noreply.github.com>
Co-authored-by: Lukas <llukas.tyla@gmail.com>
Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 15, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 15, 2024
@alexfauquette
Copy link
Member Author

Based on the styled components it would seem that we expect the theme to be overridden, but it's not doable at the moment.

I mostly use them to get access to the theme, to pick colors. And for root components to support sx props

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! 👏 🚀

@alexfauquette alexfauquette merged commit d246a1f into mui:next Feb 15, 2024
17 checks passed
@oliviertassinari oliviertassinari added the new feature New feature or request label Feb 15, 2024
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool nice to see this land 👍 .

Ideas on things that developers might raise going forward:

Comment on lines +79 to +83
/**
* The height of the chart in px. If not defined, it takes the height of the parent element.
* @default undefined
*/
height: PropTypes.number,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment moved to mui/material-ui#41139

@alexfauquette
Copy link
Member Author

Ideas on things that developers might raise going forward

It's done on purpose. I'm waiting for their feedback to prioritise

  • Animation: I plan to use slots to replace the current arcs with an animated one
  • Component behavior when the value goes outside of the [0, 100] range: That's very weird
  • Support areas (red, green, yellow) metabase.com/docs/latest/questions/sharing/visualizations/gauge: Will be the opportunity to do it also for line chart
  • A built-in GaugePointer component that can be composed with the Gauge?: Not sure. You could have so many design for pointer that the demo could be the simplest way to let them customize it
  • The docs says to use value={null} to not show the value, but it shows NaN: Can be customized with the text props. I had no idea about who to inform users that there is no value

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 16, 2024

metabase.com/docs/latest/questions/sharing/visualizations/gauge

@alexfauquette I was almost going to use this for a finance use case (showing how much cash is in our EUR bank account, and triggering an error email if it's too low https://mui.metabaseapp.com/question/305-finance-qonto-insufficient-funds) but Metabase doesn't support this Gauge charts for email reports so I used another chart that is supported (linear gauge) 😅

Not sure. You could have so many designs for pointer that the demo could be the simplest way to let them customize it

Maybe Toolpad pro code API could have a default opinionated design which is fair enough for MUI X.

Can be customized with the text props. I had no idea about who to inform users that there is no value

Maybe with , I guess the use case is to have a skeleton as the data is loading.

SCR-20240216-rghb

But en empty string '' might even be better.

@TheOneTheOnlyJJ
Copy link

  • A built-in GaugePointer component that can be composed with the Gauge?: Not sure. You could have so many design for pointer that the demo could be the simplest way to let them customize it

I suggest there be at least 1 built-in pointer (or maybe 2), making the DX as smooth as possible, and assuring the dev that Material Design principles are properly followed, unlike potential custom-made solutions.
This would standardize the look of most MUI gauge pointers around the entire web (if provided with a good-looking default option, how many devs would really make their own?).

As an example, with a single customizable pointer, it would be possible to implement both a pointer that starts from the center of the circle, like the one currently illustrated in the docs, and a pointer that consists of a tiny indicator near the inner arc border itself, like this one: https://ft.syncfusion.com/featuretour/essential-js2/images/circular-gauge/javascript-circular-gauge-redndered-with-marker-pointer.jpg

For more customization, developers would be able to create custom pointers just like suggested in the docs as of now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[charts] Add Gauge components
8 participants