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 domainLimit to axis config #15294

Merged
merged 11 commits into from
Nov 7, 2024
Merged

Conversation

GuillaumeMeheut
Copy link
Contributor

@GuillaumeMeheut GuillaumeMeheut commented Nov 6, 2024

Fix #13316

@alexfauquette Not sure if thats all it take ? Should I add docs?
Is it useful to add it to axisX aswell or should only be axisY?
Also why here #13316 (comment) you wanted to add function since we already have min/max on the axis, supposed to replace it ?

Preview: https://deploy-preview-15294--material-ui-x.netlify.app/x/react-charts/axis/#relative-axis-sub-domain

@michelengelen michelengelen added enhancement This is not a bug, nor a new feature component: charts This is the name of the generic UI component, not the React module! labels Nov 6, 2024
@mui-bot
Copy link

mui-bot commented Nov 6, 2024

Copy link

codspeed-hq bot commented Nov 6, 2024

CodSpeed Performance Report

Merging #15294 will not alter performance

Comparing GuillaumeMeheut:skip-nice (d644ae0) with master (f55e159)

Summary

✅ 3 untouched benchmarks

@alexfauquette
Copy link
Member

Also why here you wanted to add function since we already have min/max on the axis, supposed to replace it ?

It's to simplify user life. for example if you just want to start/end at a multiple of 10 you could do something like

([min, max]) => [ min - (min % 10), max + 10 - (max % 10)]

Effectively user could do the same with min/max but they will have to find by themself what is the min/max of their data. Since we already did the computation we can let them reuse the result with a function

It replace the 'dataMin', 'dataMax' in recharts domain.

@alexfauquette
Copy link
Member

Is it useful to add it to axisX aswell or should only be axisY ?

I assume they both work the same way since the processing of axis config is the same for x and y. We don't talk about it in the docs because the spark line uses band/point scale which are not impacted by the nice()

The failing CI is because you need to run pnpm docs:api to update the content of API pages

What about adding a demo in sparline docs such that other user can know about this new feature https://github.com/mui/mui-x/blob/master/docs/data/charts/sparkline/sparkline.md

It could be a subsection of "Y-axis range".

To add a demo, create a .tsx file in the same folder as the markdown, and you can run pnpm docs:typescript:formatted to generate the .js version

@GuillaumeMeheut
Copy link
Contributor Author

Effectively user could do the same with min/max but they will have to find by themself what is the min/max of their data. Since we already did the computation we can let them reuse the result with a function

Make sense now, thanks for explanation also I prioritize the domain limit function over min-max hope its okay

Also added example in docs

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.

It's good for me @JCQuintas Do you want to have a second look, especially to the docs aspect

docs/data/charts/sparkline/CustomDomainYAxis.js Outdated Show resolved Hide resolved
docs/data/charts/sparkline/CustomDomainYAxis.js Outdated Show resolved Hide resolved
@alexfauquette alexfauquette changed the title [charts] Add domainLimit to skip nice [charts] Add domainLimit to axis config. Nov 6, 2024
@alexfauquette
Copy link
Member

alexfauquette commented Nov 7, 2024

@JCQuintas @GuillaumeMeheut I moved the detail explanation into the axis page, and kept a smaller one in the sparkling.

After some test, I think having a select to chose between the different option make it simpler to see the differences between the values of domainLimit instead of showing all of them at the same time.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 7, 2024
Copy link

github-actions bot commented Nov 7, 2024

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

@JCQuintas JCQuintas enabled auto-merge (squash) November 7, 2024 15:11
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 7, 2024
@JCQuintas
Copy link
Member

@alexfauquette should we cherry pick this into v7?

@alexfauquette
Copy link
Member

We could, but If we have conflict, it might not worths resolving them

@alexfauquette alexfauquette added the needs cherry-pick The PR should be cherry-picked to master after merge label Nov 7, 2024
@JCQuintas JCQuintas added the v7.x label Nov 7, 2024
@JCQuintas JCQuintas merged commit a62b88f into mui:master Nov 7, 2024
19 checks passed
Copy link

github-actions bot commented Nov 7, 2024

Cherry-pick PRs will be created targeting branches: v7.x

github-actions bot pushed a commit that referenced this pull request Nov 7, 2024
@oliviertassinari oliviertassinari changed the title [charts] Add domainLimit to axis config. [charts] Add domainLimit to axis config Nov 7, 2024
@@ -58,3 +58,11 @@ The following demo shows two sparklines, one with small and another with large v
The first row has the default y-axis values, while on the second row a fixed range from `0` to `100` has been set.

{{"demo": "CustomYAxis.js"}}

You can adjust the y-axis range of a sparkline relatively to its data by using the `domainLimit` option in the `yAxis` configuration.
See the [axis docs page](http://localhost:3001/x/react-charts/axis/#relative-axis-sub-domain) form more information.
Copy link
Member

@oliviertassinari oliviertassinari Nov 7, 2024

Choose a reason for hiding this comment

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

Broken link and typo

Suggested change
See the [axis docs page](http://localhost:3001/x/react-charts/axis/#relative-axis-sub-domain) form more information.
See the [axis docs page](/x/react-charts/axis/#relative-axis-subdomain) for more information.

* Defines the axis scale domain based on the min/max values of series linked to it.
* - 'nice': Rounds the domain at human friendly values.
* - 'strict': Set the domain to the min/max value provided. No extras space is added.
* - function: eceives the calculated extremums as parameters, and should return the axis domain.
Copy link
Member

@oliviertassinari oliviertassinari Nov 7, 2024

Choose a reason for hiding this comment

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

Suggested change
* - function: eceives the calculated extremums as parameters, and should return the axis domain.
* - function: Receives the calculated extremums as parameters, and should return the axis domain.

</TextField>
<Typography>y-axis range: {yRange[domainLimitKey]}</Typography>
</Stack>

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +7 to +8
import { Theme } from '@mui/material/styles';
import { SparkLineChart } from '@mui/x-charts/SparkLineChart';
Copy link
Member

Choose a reason for hiding this comment

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

To avoid the blank line in the JavaScript generated demo?

Suggested change
import { Theme } from '@mui/material/styles';
import { SparkLineChart } from '@mui/x-charts/SparkLineChart';
import { SparkLineChart } from '@mui/x-charts/SparkLineChart';
import { Theme } from '@mui/material/styles';

@@ -86,6 +86,20 @@ xAxis={[

{{"demo": "MinMaxExample.js"}}

### Relative axis sub domain
Copy link
Member

Choose a reason for hiding this comment

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

?

Suggested change
### Relative axis sub domain
### Relative axis subdomain

<MenuItem value="strict">strict</MenuItem>
<MenuItem value="function">function</MenuItem>
</TextField>

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

import { SparkLineChart } from '@mui/x-charts/SparkLineChart';

const settings = {
valueFormatter: (v: number | null) => `${v}%`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
valueFormatter: (v: number | null) => `${v}%`,
valueFormatter: (value: number | null) => `${value}%`,

import MenuItem from '@mui/material/MenuItem';

const settings = {
valueFormatter: (v: number | null) => `${v}%`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
valueFormatter: (v: number | null) => `${v}%`,
valueFormatter: (value: number | null) => `${value}%`,

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 needs cherry-pick The PR should be cherry-picked to master after merge v7.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[charts] Allow basic customization of Sparkline y-axis
6 participants