-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts] Add tick spacing property #20282
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 tick spacing property #20282
Conversation
|
Deploy preview: https://deploy-preview-20282--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
fb049d5 to
b8a212f
Compare
| */ | ||
| tickLabelPlacement?: 'middle' | 'tick'; | ||
| /** | ||
| * The space between ticks when using an ordinal scale. It defines the minimum distance in pixels between two ticks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we only apply this to ordinal scales?
| tickLabelPlacement?: 'middle' | 'tick'; | ||
| /** | ||
| * The space between ticks when using an ordinal scale. It defines the minimum distance in pixels between two ticks. | ||
| * @default 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to increase the default in v9? Linear scales indirectly use 50 for this (because it calculates the default tick number as range[1] - range[0] / 50)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wouldn't increase it either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @default 0 |
| */ | ||
| tickLabelPlacement?: 'middle' | 'tick'; | ||
| /** | ||
| * The space between ticks when using an ordinal scale. It defines the minimum distance in pixels between two ticks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to document how this interacts/conflicts with tickMinStep, tickMaxStep, tickLabelInterval and tickLabelMinGap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to document how this interacts/conflicts with tickMinStep, tickMaxStep, tickLabelInterval and tickLabelMinGap
For now it does not interact, because those are for continuous scale only (except for tickInterval)
What about
The minimum distance in pixels between two ticks.
- For ordinal scale, ticks are filtered out to match the distance.
- For continuous scale, the
tickSpacingis used to compute thetickumberif not defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For continuous scale, the tickSpacing is used to compute the tickumber if not defined.
For continuous scales we're defaulting this to 50, so it would only work if we change the default depending on the scale type.
b8a212f to
6a97e6a
Compare
|
@alexfauquette your input would be especially appreciated here as you've worked on a similar topic 😄 |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
f47a446 to
fb0a165
Compare
CodSpeed Performance ReportMerging #20282 will not alter performanceComparing Summary
Footnotes |
5336655 to
7824e1b
Compare
82239a0 to
2f4974c
Compare
alexfauquette
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior looks good, and it's a nice move in direction of more resposive charts.
Sorry for the delay in reviewing this PR
| const every = Math.ceil(domain.length / (rangeSpan / tickSpacing)); | ||
| return domain.filter((_, index) => index % every === 0); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To handle the case tick spacing = 0
| const every = Math.ceil(domain.length / (rangeSpan / tickSpacing)); | |
| return domain.filter((_, index) => index % every === 0); | |
| } | |
| const every = Math.ceil(domain.length * tickSpacing / rangeSpan); | |
| if(Number.isNaN(every) || every <= 1){ | |
| return domain; | |
| } | |
| return domain.filter((_, index) => index % every === 0); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stated that assumption in the documentation of the function:
Assumes tick spacing is greater than 0.
Do we need to handle it? It should already be ensured by the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It cost nothing, plus add an extra savings:
- avoid to run
filterif theeveryis equal to 1 - do no crash
rangeSpanis 0
| */ | ||
| tickLabelPlacement?: 'middle' | 'tick'; | ||
| /** | ||
| * The space between ticks when using an ordinal scale. It defines the minimum distance in pixels between two ticks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to document how this interacts/conflicts with tickMinStep, tickMaxStep, tickLabelInterval and tickLabelMinGap
For now it does not interact, because those are for continuous scale only (except for tickInterval)
What about
The minimum distance in pixels between two ticks.
- For ordinal scale, ticks are filtered out to match the distance.
- For continuous scale, the
tickSpacingis used to compute thetickumberif not defined.
| tickLabelPlacement?: 'middle' | 'tick'; | ||
| /** | ||
| * The space between ticks when using an ordinal scale. It defines the minimum distance in pixels between two ticks. | ||
| * @default 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d1e85a5 to
dd7136f
Compare
alexfauquette
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor comments to polish
| tickLabelPlacement?: 'middle' | 'tick'; | ||
| /** | ||
| * The space between ticks when using an ordinal scale. It defines the minimum distance in pixels between two ticks. | ||
| * @default 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @default 0 |
| filteredDomain = filteredDomain.filter(tickInterval); | ||
| } | ||
|
|
||
| if (tickSpacing !== undefined && tickSpacing > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (tickSpacing !== undefined && tickSpacing > 0) { | |
| if (tickSpacing) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the current way of writing more explicit, otherwise it might not be clear that this isn't an oversight. I can replace it with a comment if you prefer, though
| tickLabelInterval?: 'auto' | ((value: any, index: number) => boolean); | ||
| /** | ||
| * The minimum space between ticks when using an ordinal scale. It defines the minimum distance in pixels between two ticks. | ||
| * @default 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @default 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we remove the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why adding a default instead of just considering that if not provided it has no effect?
Plus it will stay accurate if at some point we extend the impact of this property to continuous axes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just considering that if not provided it has no effect
Should we state that instead, then? Otherwise I don't think it's clear for users what the default value is.
If they're looking at this prop for the first time, isn't it plausible that we set a default value different than 0?
docs/data/charts/axis/axis.md
Outdated
| You can use the `tickSpacing` property to define the minimum spacing in pixels between two ticks in ordinal scales. | ||
|
|
||
| By default, this value is set to 0, so there is no minimum spacing between ticks. | ||
|
|
||
| For band scales where hidden ticks can be interpolated, such as numbers and dates, the tick spacing can be set to a positive value. | ||
|
|
||
| This should improve the readability of the axis by reducing the number of ticks shown as well as improve performance when there is a large number of ticks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| You can use the `tickSpacing` property to define the minimum spacing in pixels between two ticks in ordinal scales. | |
| By default, this value is set to 0, so there is no minimum spacing between ticks. | |
| For band scales where hidden ticks can be interpolated, such as numbers and dates, the tick spacing can be set to a positive value. | |
| This should improve the readability of the axis by reducing the number of ticks shown as well as improve performance when there is a large number of ticks. | |
| Use the `tickSpacing` property to define the minimum spacing in pixels between two ticks. | |
| Having a minimal space between ticks improves the readability and the performances of the axis. | |
| This property is only available for ordinal axes (the one with band and point scale). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rewritten some of this paragraph, but why do you think we should remove the usage suggestion and the default value? I think it's useful info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think documentation readers have a small capacity for attention.
- The explanation on the default is a full sentence to explain what happen if they don't set this property. But they already have their code running, or they have seen dozens of bar chart before so they intuitively know there is a tick per category by default
- About the usage, I've not removed it. I only focused on what I think matters the most:
- It's interesting for readability and performances 👍
- The fact that values should be interoperable: That's a concern for us, not for the users. They have a particular dataset to display, so they intuitively know if they can remove ticks or not.
- insist on the fact it only works on ordinal scales, to be sure people don't lose their mind trying to make this work on a continuous one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I tried shortening the docs even further


Part of #12960.
Adds a
tickSpacingproperty that defines the minimum space in pixels between ticks. At the moment, this is only used for ordinal scales. This should improve performance significantly while also improving the look of ticks in bar charts with many data points.Ideally, in v9 we'd set this default to 50px, similar to continuous scales.
Demo