Skip to content

[charts] Fix infinite tick number when zoom range is zero #17750

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

Merged
merged 2 commits into from
May 9, 2025

Conversation

bernardobelchior
Copy link
Member

@bernardobelchior bernardobelchior commented May 8, 2025

Fix infinite tick number when zoom range is zero.

This was causing a crash when the zoom range is zero (i.e., start === end) because d3's scale.ticks creates an Array(tickNumber) which throws when tickNumber is Infinity.

This might happen if the user sets minSpan to 0 or calls setZoomData directly.

Copy link

github-actions bot commented May 8, 2025

Thanks for adding a type label to the PR! 👍

@bernardobelchior bernardobelchior added bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module! labels May 8, 2025
@mui-bot
Copy link

mui-bot commented May 8, 2025

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

Generated by 🚫 dangerJS against cdfe045

@@ -200,7 +200,9 @@ export function computeAxisValue<T extends ChartSeriesType>({
}

const rawTickNumber = getTickNumber({ ...axis, range, domain: axisExtremums });
const tickNumber = rawTickNumber / ((zoomRange[1] - zoomRange[0]) / 100);
/* If the zoom start and end are the same, `tickNumber` will become infinity, so we should default to 1. */
const tickNumber =
Copy link
Member Author

Choose a reason for hiding this comment

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

Decided to default to one tick because if zoom range is zero, there is no need to show more than one tick.

Copy link
Member

Choose a reason for hiding this comment

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

Should we have a test for this? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Added 👍

Copy link

codspeed-hq bot commented May 8, 2025

CodSpeed Performance Report

Merging #17750 will not alter performance

Comparing bernardobelchior:fix-zero-zoom (cdfe045) with master (e2dceb2)

Summary

✅ 8 untouched benchmarks

@bernardobelchior bernardobelchior marked this pull request as ready for review May 8, 2025 10:25
@@ -46,24 +46,6 @@ export interface TickParams {
tickLabelPlacement?: 'middle' | 'tick';
}

export function getTickNumber(
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to ticks

@bernardobelchior bernardobelchior merged commit add0d27 into mui:master May 9, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants