-
-
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
[charts] Add new bumpX
and bumpY
curve options
#16318
Conversation
Deploy preview: https://deploy-preview-16318--material-ui-x.netlify.app/ Updated pages: |
CodSpeed Performance ReportMerging #16318 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.
I'm ok to add them. I just left one concern about the docs
For a first version of the funnel I don't see that as necessary. Some straight lines could do the job
'bumpY', | ||
'bumpX', | ||
]; |
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 would be in favor of not documenting those options. I can see the interest for drawing a funnel chart but not a line chart. So I would not bother reader with additional option they might never use
"labelMarkType": { | ||
"description": "Defines the mark type for the series.<br /><br />There is a default mark type for each series type.<br /><br />It allows custom values which will be passed to the mark component if it was customized." | ||
"description": "Defines the mark type for the series.<br /><br />There is a default mark type for each series type." | ||
}, |
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.
Did something change about how you see them used in the future?
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.
It is just not implemented right now
Curves were already implemented for a long time, I'm just extracting parts of the Funnel PR to make it smaller :) |
bumpX
andbumpY
curve options as they are better to use withfunnel