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 baseline property to the LineChart series #14153

Merged
merged 14 commits into from
Aug 12, 2024

Conversation

JCQuintas
Copy link
Member

Resolves #14145

This adds a baseline props to the line chart series. It only works on the area chart and defines the baseline the area starts from.

baseline: 0
Screenshot 2024-08-09 at 15 14 31

baseline: -10
Screenshot 2024-08-09 at 15 14 38

baseline: 10
Screenshot 2024-08-09 at 15 14 43

@JCQuintas JCQuintas added new feature New feature or request component: charts This is the name of the generic UI component, not the React module! labels Aug 9, 2024
@JCQuintas JCQuintas self-assigned this Aug 9, 2024
@JCQuintas JCQuintas marked this pull request as ready for review August 9, 2024 13:15
@mui-bot
Copy link

mui-bot commented Aug 9, 2024

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

Updated pages:

Generated by 🚫 dangerJS against bb563f4

Copy link

codspeed-hq bot commented Aug 9, 2024

CodSpeed Performance Report

Merging #14153 will not alter performance

Comparing JCQuintas:area-chart-baseline (bb563f4) with master (38184fe)

Summary

✅ 3 untouched benchmarks

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.

Just one edge case. I think the second option will be better when used with the zoom feature you're currently working on

docs/data/charts/lines/lines.md Outdated Show resolved Hide resolved
Comment on lines 101 to 104
if (typeof baseline === 'number') {
return yScale(baseline)!;
}

Copy link
Member

Choose a reason for hiding this comment

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

This only impacts the plotting. Here is an example with values fluctuating between 200-300, and with baseline set to 1000.

Notice that:

  • The basline has no impact on the max value of the y-axis
  • The yèaxis always start at 0 because it's the assumed baseline

image

Copy link
Member

Choose a reason for hiding this comment

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

Potential solution

  1. When computing the extremum values, you replace when needed the base (d[0]) by the user defined baseline

  2. Instead of providing a number, user can provide 'top' | 'bottom'. Then you just need to ignore the d[0] by doing

- isArea && axis.scaleType !== 'log' ? ...
+ isArea && axis.scaleType !== 'log' && series[seriesId].baseline === undefined ? ...

and in the commented line return

- return yScale(baseline)!;
- return baseline==='top' ? yScale.range[0] : yScale.range[1];

const { area, stackedData } = series[seriesId];
const isArea = area !== undefined;

Copy link
Member Author

Choose a reason for hiding this comment

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

It behaves a bit weirdly when baseline changes the extremums.

This happens because the extremum getter value is then passed into the nice() function, it can have unintended consequences, which means I would also have to check the baseline where that is calculated.

But we already have the axis min/max values for that. So I believe we should use the min/max values when the axis extremums need to be changed.

screencapture-localhost-3001-playground-area-chart-baseline-2024-08-12-11_20_10

Copy link
Member

Choose a reason for hiding this comment

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

It behaves a bit weirdly when baseline changes the extremums.

TLTR: I don't think it's an issue


Technically speaking, we have two behavior to define:

  • where does the area set the baseline
  • should the basline impact the extremums.

Should the baseline impact the extremums

For stacked data, I can not think of a usecase where the answer would be no. Otherwise you would compare some area wherase the base one would have missing part.

image

For a single series, where the area is more aesthetic, seems it can be ignored. Here foe example the NASDAQ evolution during the day. Nobody care of the space between 0 and 16.500. Only the relative evolution matter. Otherwise you would see nothing.

image

Where does the area set the baseline

I only see two reasons to modify the default 0:

  • aesthetic reasons. But in that case, only min/max makes sense
  • There is a specific value to compare to. For example the average over time. But in that case, I don't see a reason to pick a value that is always on top or below the line

So basically the weird case you define should not exist

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, but those are just working as expected. The issue that appears when we have the baseline affect the extremums is that the extremums will get niced after. So if we have a non-round number like below, it will not work properly. Then this starts to get more complex and would clash with a feature that already exists yAxis.min/max

Screenshot 2024-08-12 at 13 03 52

JCQuintas and others added 5 commits August 12, 2024 09:55
Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
packages/x-charts/src/LineChart/extremums.ts Outdated Show resolved Hide resolved
packages/x-charts/src/models/seriesType/line.ts Outdated Show resolved Hide resolved
docs/data/charts/lines/lines.md Outdated Show resolved Hide resolved
docs/data/charts/lines/AreaBaseline.tsx Outdated Show resolved Hide resolved
Comment on lines 186 to 188
:::info
Keep in mind the baseline only affects the area visuals. If you want to change the axis minimum or maximum values, you should use the `min/max` property of the axis.
:::
Copy link
Member

@alexfauquette alexfauquette Aug 12, 2024

Choose a reason for hiding this comment

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

Not exactly if the baseline means we will ignore 0 for extremum computation

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 don't get what you mean here 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I forgot a part of the sentence

If the baseline is defined with min/max, the 0 will be ignored for axis extremums

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this is an effect of your change, I didn't intend it at first. We can keep to the other discussion and I will update it with the outcomes

JCQuintas and others added 5 commits August 12, 2024 12:40
Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
@JCQuintas JCQuintas changed the title [charts] Implement baseline for area charts [charts] Add baseline property to the LineChart series Aug 12, 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.

Just one last comment about the impact on stacked line chart. Since you early return the y0 function, the behavior with stacked line could be wrong. Just warning users about that can do the job

Comment on lines 184 to 187
It is also possible to provide a `number` value to fix the baseline at the desired position.

{{"demo": "AreaBaseline.js"}}

Copy link
Member

Choose a reason for hiding this comment

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

You could add a warning to say that the baseline should not be used with stacked lines

@JCQuintas JCQuintas merged commit d5f2c0f into mui:master Aug 12, 2024
19 checks passed
@JCQuintas JCQuintas deleted the area-chart-baseline branch August 12, 2024 14:54
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] Setting baseline for Area Chart
3 participants