-
-
Notifications
You must be signed in to change notification settings - Fork 314
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.js] Check if chart is already connected #1944
base: 2.x
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
@smnandre will you be able to merge this or is something missing? |
Hi @Shadow-Devil! Sorry we've all been very busy on our "day lifes".. II have a doubt here i'd like the opinion on others.. This situation (a chart already initialized on the element) should not happen, right ? So i totally agree we should add a early exit there, but.... should we throw an error ? |
@smnandre Sorry, I didn't want to sound rude😅.
This situation happened for me, because of Google's Material Design Lite which would disconnect and reconnect the chart from the DOM. A second solution would be to call |
Hey, either throwing the error or exiting silently won't really fix the initial issue right? Throwing an error looks better to me, because it will really help the developer to understand that something wrong is happening. But, if the Chart is previously initialized and WDYT? |
Everytime, or in particular scenarios ? It feels a bit strange :| --
That's my opinion too: in this scenario, throwing an Error after an early exit seems the correct thing to do (this PR + an error) --
This feels a good thing to do yes! (there may be a reason why it has not be done before ( 🤷 )) (thanks @Kocal) |
Once on page reload.
The connect method can be called multiple times by stimulus. See also the stimulus docs about lifecycle callbacks:
I will try to update the PR to destroy the chart on disconnect 👍🏻 |
Didn't know about that, thanks :) |
Only after beeing deconnected. So if the Chart.js instance is destroyed this would not create problem right ? |
Hm, so calling |
In your case i understand this can be not perfect... (but still don't understand why your framework deconnect then reconnect div on page load) But to me, it seems logical to use connect there, because it can happen multiple times. That makes me think.... is this not the reason there is no disconnect: https://ux.symfony.com/demos/live-component/chartjs Would this demo still work if we did disconnect every time? (genuine question). Let's continue tomorrow ! |
Adds an extra check if the chart is already connected and short circuits, since otherwise creating the new Chart would throw an error.