-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Use isPointInside
function for both graphs and axis
#14222
Conversation
JCQuintas
commented
Aug 15, 2024
- This normalises the code to use the same logic in both instances.
- It also fixes an issue where the axis would show before the datapoint, even though the were displayed in the same line.
Deploy preview: https://deploy-preview-14222--material-ui-x.netlify.app/ |
CodSpeed Performance ReportMerging #14222 will not alter performanceComparing Summary
|
const showTick = isPointInside({ x: offset }, { direction: 'x' }); | ||
const showTickLabel = isPointInside({ x: offset + xTickLabel }, { direction: 'x' }); |
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'm not sure if it matters much, but the shape { x?: number, y:? number }
is un-monomorphizing the isPointInside
function. If it's a hot function, could be worth to pass a placeholder value like { x: offset, y: -1 }
.
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.
LGTM. Feel free to implement Romain proposal. It's true that this function might be called a lot