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] Move Voronoi handler in a dedicated plugin #16470

Merged
merged 9 commits into from
Feb 11, 2025

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Feb 5, 2025

Remove the Voronoi handler and make it aplugins.

In addition, we only compute the delauney graph when the zoom is not active

@mui-bot
Copy link

mui-bot commented Feb 5, 2025

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

github-actions bot commented Feb 6, 2025

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 7, 2025
* @param scale The scale to use
* @returns (value: any) => number
*/
export function getValueToPositionMapper(scale: D3Scale) {
Copy link
Member Author

Choose a reason for hiding this comment

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

extracted from the useScale.ts

Comment on lines +42 to +46
export const selectorChartZoomIsInteracting = createSelector(
selectorChartZoomState,
(zoom) => zoom?.isInteracting,
);

Copy link
Member Author

Choose a reason for hiding this comment

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

The selector got moved to the communit packeg to be able to stop voronoi computation during the zoom

@alexfauquette alexfauquette added breaking change component: charts This is the name of the generic UI component, not the React module! labels Feb 7, 2025
@alexfauquette alexfauquette marked this pull request as ready for review February 7, 2025 10:05
Copy link

codspeed-hq bot commented Feb 7, 2025

CodSpeed Performance Report

Merging #16470 will not alter performance

Comparing alexfauquette:delauney (2338b4b) with master (3f8343a)

Summary

✅ 6 untouched benchmarks

@JCQuintas
Copy link
Member

Tests are failing because of changes in this PR

@alexfauquette
Copy link
Member Author

Tests are failing because of changes in this PR

Nice it also helped me to realise I messed a bit with the removeItemInteraction I should add tests on it (I created #16513 for that purpose)

Copy link
Member

@JCQuintas JCQuintas left a comment

Choose a reason for hiding this comment

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

One test is still failing. I couldn't figure out why it is failing though. The code examples seem to behave correctly.

@alexfauquette
Copy link
Member Author

The remaining failing test came from the fact that when voronoi is enabled it should be the source of truth an so pointerEnter should not trigger interaction.

Seems the behavior was buggy an fixed by this PR.

Investigation let me also noticed one prop was not properly propagated. So I added a test on scatter chart such that the tooltip got test for both voronoi and item interacion

Comment on lines +74 to +77
fireEvent.pointerMove(svg, {
clientX: 10,
clientY: 10,
}); // Set tooltip position voronoi value
Copy link
Member

Choose a reason for hiding this comment

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

Odd that this was necessary 🤔

I suppose it is because it is using fireEvent instead of userEvent, which would handle moving/hover/etc

@alexfauquette alexfauquette merged commit 5eeb07f into mui:master Feb 11, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change 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