-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[chart] Use default values instead of non-null assertion to prevent error being thrown #13637
Conversation
Deploy preview: https://deploy-preview-13637--material-ui-x.netlify.app/ |
return ( | ||
<React.Fragment> | ||
{xAxisHighlight === 'band' && axis.x !== null && isBandScale(xScale) && ( | ||
<ChartsAxisHighlightPath | ||
d={`M ${xScale(axis.x.value)! - (xScale.step() - xScale.bandwidth()) / 2} ${ | ||
d={`M ${(xScale(axis.x.value) ?? 0) - (xScale.step() - xScale.bandwidth()) / 2} ${ |
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.
If xScale(axis.x.value)
is not defined, Maybe it would make more sense not to render this component instead of placing it at coordinate 0
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 went with rendering at 0 because it would be clearer that something is wrong, rather than silently failing. Though I don't have a strong preference
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.
What about logging a warning?
This should not append, but I think it's better to have a non working features that logs in the console why it's not working
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.
🤔 Wait I did something wrong 😅
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.
fixed 👍
return ( | ||
<React.Fragment> | ||
{xAxisHighlight === 'band' && axis.x !== null && isBandScale(xScale) && ( | ||
<ChartsAxisHighlightPath | ||
d={`M ${xScale(axis.x.value)! - (xScale.step() - xScale.bandwidth()) / 2} ${ | ||
d={`M ${(xScale(axis.x.value) ?? 0) - (xScale.step() - xScale.bandwidth()) / 2} ${ |
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.
What about logging a warning?
This should not append, but I think it's better to have a non working features that logs in the console why it's not working
Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com> Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
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.
Just a nitpick. Otherwise it's a nice refactoring 👍
packages/x-charts/src/ChartsAxisHighlight/ChartsAxisHighlight.tsx
Outdated
Show resolved
Hide resolved
…rror being thrown (mui#13637) Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com> Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
…rror being thrown (mui#13637) Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com> Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Some cleanup related to #13405 but moved here to make that PR cleaner