-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
ensure getComputedStyle
always has a valid canvas provided
#11809
ensure getComputedStyle
always has a valid canvas provided
#11809
Conversation
@etimberg / @LeeLenaleee I used optional chaining and null coalescing because of this discussion - #11651 (comment), which is why the CI pipeline failed. Give the word and I'll remove the rule so this pipeline can pass. |
* @param event | ||
* @param chart |
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 guess that this was not meant to be?
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.
this was removed since it didn't provide any additional information than the function signature already provided (not even variable types). If you still want this, I can revert this change.
@etimberg / @LeeLenaleee can you re-review this? |
It feels kind of hacky to just return semi default values if the canvas does not exist. Can you reproduce the errors because it seems to me that if you remove the parents and keep chart.js alive. But if you destroy the chart first these errors also should not happen |
The canvas probably got unmounted as a result of React's rerendering (I was using Chartjs via https://github.com/reactchartjs/react-chartjs-2). Since we've hit enough issues with Chartjs in our react application, my team has decided to move away from chartjs. Feel free to close or update this pull request as you see fit. |
Expected behavior
getComputedStyle()
fromhelpers.dom.js
should be called with validcanvas
values provided.Current behavior
From sentry:
Reproducible sample
.
Optional extra steps/info to reproduce
No response
chart.js version
4.4.3
Browser name and version
Experienced on Chrome >=124.0.0 according to my project's Sentry, but probably impacts others since this is an issue with the provided parameter.
Link to your project
No response