Skip to content

[charts] Add a PolarProvider to manage polar axes#14642

Merged
alexfauquette merged 6 commits intomui:masterfrom
alexfauquette:add-polar-coordinate
Sep 18, 2024
Merged

[charts] Add a PolarProvider to manage polar axes#14642
alexfauquette merged 6 commits intomui:masterfrom
alexfauquette:add-polar-coordinate

Conversation

@alexfauquette
Copy link
Copy Markdown
Member

@alexfauquette alexfauquette commented Sep 16, 2024 •
edited
Loading

Follow-up of #14641

It introduces a PolarProvider which has similar behavior has the cartesian one. The main differences:

  • The range computation:
    • The rotation goes from startAngle to endAngle
    • The radius goes from minRadius to maxRadius
  • No Zoom
  • Of course the naming rotation/radius instead of x/y axes

Note the provider is not exported. So this is still internal code

@alexfauquette alexfauquette added the scope: charts Changes related to the charts. label Sep 16, 2024
@mui-bot
Copy link
Copy Markdown

mui-bot commented Sep 16, 2024 •
edited
Loading

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

Generated by 🚫 dangerJS against 68a729e

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Sep 17, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #14642 will not alter performance

Comparing alexfauquette:add-polar-coordinate (68a729e) with master (c93f76b)

Summary

✅ 3 untouched benchmarks

@alexfauquette alexfauquette marked this pull request as ready for review September 17, 2024 07:48
JCQuintas
JCQuintas approved these changes Sep 17, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Nice work 👍, left a comment in code plus the observation below

There is an empty file at packages/x-charts/src/context/PolarProvider/defaultizeAxis.ts

Comment thread packages/x-charts/src/context/PolarProvider/computeValue.ts Outdated
@alexfauquette
Copy link
Copy Markdown
Member Author

alexfauquette commented Sep 17, 2024

I merged the two computeValue in one. It's not supper coherent because the function is use in cartesian and polar context, plus exported as a cartesian axis utils. But should be good enough for now. I'm not sure about how much we will use this PolarProvider outside of the radar

@JCQuintas
Copy link
Copy Markdown
Member

JCQuintas commented Sep 17, 2024

I merged the two computeValue in one. It's not supper coherent because the function is use in cartesian and polar context, plus exported as a cartesian axis utils. But should be good enough for now. I'm not sure about how much we will use this PolarProvider outside of the radar

I think the changes are small enough that makes sense to have a single logic.

In regards to where to put it, we could move it into the internals folder maybe?

@alexfauquette alexfauquette merged commit 2889da1 into mui:master Sep 18, 2024
@alexfauquette alexfauquette deleted the add-polar-coordinate branch September 18, 2024 08:30
arthurbalduini pushed a commit to arthurbalduini/mui-x that referenced this pull request Sep 30, 2024
@alexfauquette
[charts] Add a PolarProvider to manage polar axes (mui#14642)
e8bc9a9
This was referenced Oct 25, 2024
Open
Open
Open
This was referenced Nov 1, 2024
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JCQuintas JCQuintas JCQuintas approved these changes

Assignees

No one assigned

Labels

scope: charts Changes related to the charts.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alexfauquette @mui-bot @JCQuintas