Skip to content
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

Add destroyDelay prop #1085

Merged
merged 2 commits into from Apr 9, 2024
Merged

Conversation

doutatsu
Copy link
Contributor

@doutatsu doutatsu commented Feb 29, 2024

Fixes #579

The original issue was marked as completed, but it was not fixed. To address this issue, I've added a new prop that delays calling destroy on unMount. No matter how long your transition is, you can adjust the charts as a result

Before

chart-bug.mp4

With delay

graph-fixed.mp4

@apertureless
Copy link
Owner

Hey! Thanks for the PR. Honestly I thought this was fixed a while ago.
Reading the initial issue, as far as I understand the problem is, that if you are using vue-router and have transitions for the page changes, the charts get destroyed too early.

It would be good to have a reproduction for this. However, I do not think that adding a setTimeout is the proper way here.

I think it would be easier and cleaner to destroy the chart on the onUnmounted hook instead of onBeforeUnmount https://github.com/doutatsu/vue-chartjs/blob/allow-destroy-delay/src/chart.ts#L70

But this needs some testing if it is really solving the issue.

@doutatsu
Copy link
Contributor Author

doutatsu commented Mar 1, 2024

@apertureless That's a good point, thank you for a quick response. I'll see if simply using a different lifecycle hook will do the trick and if so, update this PR to reflect the changes

@doutatsu
Copy link
Contributor Author

doutatsu commented Mar 4, 2024

@apertureless It doesn't seem to do the trick unfortunately - it still gets destroyed too quickly, unlike when I use a simple delay. I've added videos to demonstrate the problem and fix

@doutatsu
Copy link
Contributor Author

doutatsu commented Apr 5, 2024

@apertureless Any updates on this?

@apertureless apertureless merged commit 8d19c31 into apertureless:main Apr 9, 2024
@doutatsu doutatsu deleted the allow-destroy-delay branch April 30, 2024 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Charts destroyed before router transition ends
2 participants