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] Allow SeriesValueFormatter to return null value #15057

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

clins1994
Copy link
Contributor

@clins1994 clins1994 commented Oct 22, 2024

TL;DR

A. Omit irrelevant tooltip values by updating the SeriesValueFormatter type to allow retuning null values
B. Usage: update valueFormatter function to return null if the value is irrelevant

Problem

Use case: when displaying multiple series with the same label/color on a stacked bar chart it can be useful to omit the series that have no value for a given stack.

Note: I am interested in seeing 'axis' tooltip not 'item'

const series = [
  { ... , valueFormatter: (v: number | null) => v ? `${v} 分` : '-' }
]

The screenshot below shows the tooltip of a stack that has "MOVING", "STOPPED", and "MOVING"

In this example the order of these events matter both on the x axis and y axis so it was decided to split the multiple "MOVING" data into separate series

When hovering over the tooltip it becomes polluted with irrelevant '-' values.

Screenshot 2024-10-22 at 19 22 50

I am able to filter out the values by passing null as a return but I get a type error which I was able to ignore using !

const series = [
  { ... , valueFormatter: (v: number | null) => v ? `${v} 分` : null! }
]

With this I can visualize only the data that is displayed on the chart

Screenshot 2024-10-22 at 19 22 16

Solution

Since forcing the return of null didn't break but actually fixed my issue I wrote this PR to update the type to match the already allowed and expected behavior

@mui-bot
Copy link

mui-bot commented Oct 22, 2024

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

Generated by 🚫 dangerJS against 6e5f596

@zannager zannager added the component: charts This is the name of the generic UI component, not the React module! label Oct 22, 2024
Copy link

codspeed-hq bot commented Oct 22, 2024

CodSpeed Performance Report

Merging #15057 will not alter performance

Comparing clins1994:master (6e5f596) with master (018f118)

Summary

✅ 3 untouched benchmarks

@JCQuintas
Copy link
Member

Thanks @clins1994, this seems to be a good addition. @alexfauquette do you see any issue?

@JCQuintas JCQuintas changed the title [charts] update SeriesValueFormatter type to allow returning null values [charts] update SeriesValueFormatter type to allow returning null values Nov 5, 2024
@JCQuintas JCQuintas added the enhancement This is not a bug, nor a new feature label Nov 5, 2024
@alexfauquette alexfauquette added needs cherry-pick The PR should be cherry-picked to master after merge v7.x labels Nov 5, 2024
@alexfauquette alexfauquette changed the title [charts] update SeriesValueFormatter type to allow returning null values [charts] Allow SeriesValueFormatter to return null value Nov 5, 2024
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I considered moving this to the fact that the value is null instead of the formatted one. But a user could have the opposite need: showing something in the tooltip for missing values

@alexfauquette alexfauquette merged commit f955ebb into mui:master Nov 5, 2024
26 of 27 checks passed
@clins1994
Copy link
Contributor Author

@alexfauquette @JCQuintas thank you for the reviews 🤝

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! enhancement This is not a bug, nor a new feature v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants