Skip to content

[charts] Add axis highlight to the radar #16868

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 4 commits into from
Mar 12, 2025

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Mar 7, 2025

Follow up on #16406

Main topics:

  • I did not introduce a rotation-axis entry in the interact. Instead, I reused the x-axis under the assumption that x/y is similar to rotation/radius in practice.
  • I copied the getAxisValue function even though most of the code is similar. Manipulating angles and the fact that 0° and 360° are the same will pollute the main one.

TODO before merging

  • Refine the highlight design
  • Add propose to manage highlight behavior and document it

@alexfauquette alexfauquette added type: enhancement This is not a bug, nor a new feature scope: charts Changes or issues related to the charts product labels Mar 7, 2025
Copy link

github-actions bot commented Mar 7, 2025

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

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

mui-bot commented Mar 7, 2025

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

Updated pages:

Generated by 🚫 dangerJS against c50670d

Copy link

codspeed-hq bot commented Mar 7, 2025

CodSpeed Performance Report

Merging #16868 will not alter performance

Comparing alexfauquette:radar-highlight (c50670d) with master (94ef0a4)

Summary

✅ 7 untouched benchmarks

@alexfauquette alexfauquette marked this pull request as ready for review March 11, 2025 15:35
* - slice: display a slice around the highlighted value. Recommended for radar with a single series.
* The default value is computed depending on the number of series provided.
*/
axisHighlightShape: 'points' | 'slice';
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to highlight slices? 🤔
If yes, should they be less opaque (0.9 maybe)? 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it make sense. The different configuration make it look weird. But the point look also weird for single series. That's why I ended up with the current default

@JCQuintas
Copy link
Member

  • I did not introduce a rotation-axis entry in the interact. Instead, I reused the x-axis under the assumption that x/y is similar to rotation/radius in practice.

Though it works.

In the future, we should start thinking of another name for axes in these global utilities. Main/Other or Main/Cross like css? 🤔

@alexfauquette alexfauquette merged commit d3cde4c into mui:master Mar 12, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: charts Changes or issues related to the charts product type: enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants