-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[charts] Add RadarAxis
component to render labels
#19240
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
Conversation
Deploy preview: https://deploy-preview-19240--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
CodSpeed Performance ReportMerging #19240 will not alter performanceComparing Summary
|
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.
Works nicely, left some polishing comments
textAnchor?: string | ((angle: number) => string); | ||
/** | ||
* The labels dominant baseline or a function returning the dominant baseline for a given axis angle (in degree). | ||
*/ | ||
dominantBaseline?: string | ((angle: number) => string); |
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.
Cant we provide the proper values that should be used here? string
may be too generic
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.
Not yet but I'm working on it :)
https://github.com/DefinitelyTyped/DefinitelyTyped/pull/73510/files
|
||
- `angle` The angle used to display labels. By default it's the one associated to the given metric. | ||
- `labelOrientation` The orientation strategy. Either horizontal labels with moving anchor point, or label rotating with the axis. | ||
- `divisions` The number of labels to display. |
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.
Why is this different than <RadarChart divisions/>
? Maybe also remove the "sync" in the example, as that is confusing, as changing the RadaAxis divisions shouldnt affect the grid.
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 think most of the user will have it sync. But I do not enforce it such that user can have 4 divisions and 2 labels
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.
My point was more to show them that you can have different values between them. I was confused at first when I saw that the axis had its own "divisions". It makes sense that they are separate, but the example should threat them as separate things so it is clear to user.
RadarAxis
component to render labels
Fix #18637