LG-4797: Fix charts not resizing when container dynamically changes #2660
+331
−143
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: e4ddf91 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
tsck
commented
Jan 24, 2025
| if (zoomSelect?.xAxis || zoomSelect?.yAxis) { | ||
| setTimeout(() => { | ||
| echart.enableZoom(); | ||
| }, 100); |
There was a problem hiding this comment.
I hate everything about this. 100% open to suggestions.
There was a problem hiding this comment.
lol.
Is debounce a possibility? This is rough but maybe something like this?
const handleResize = (() => {
const debounced = debounce(() => {
console.log('Resize finished!');
// Perform additional actions if needed
}, 100); // Adjust debounce delay as needed
return function () {
console.log('Resizing...');
myChart.resize(); // Adjust chart size
debounced();
};
})();There was a problem hiding this comment.
debounce would probably work, that's a good suggestion, but I guess we'd be effectively doing the same thing - delaying re-enabling zoom by some arbitrary amount. I think it would add a bit of obfuscation around it though, so I think I'd actually prefer the timeout for this case.
I think the "correct" way of doing it would be tying into the render event. At this point I feel the chart should be ready for the change, but for some reason that I can't understand, it's not =(.
I'm ok with leaving the arbitrary timeout if need be, it just creates potentially brittle code since it is arbitrary and lacks any guarantee that it will be "enough" time.
There was a problem hiding this comment.
That makes sense. What are the 'rendered' and 'finished' events you mentioned in the comment?
There was a problem hiding this comment.
I found it: https://echarts.apache.org/en/api.html#events.finished; where were you calling this?
There was a problem hiding this comment.
Right here. Instead of using the timeout, I attempted to add a callback to those events that would turn on zoom. Then like on initial render, the callback needed to be removed after so that it wouldn't create an infinite loop. So it looked like:
if (zoomSelect?.xAxis || zoomSelect?.yAxis) {
function enableZoomOnRender() {
echart.enableZoom();
echart?.off('finished', enableZoomOnRender);
}
echart?.on('rendered', enableZoomOnRender);
}I put this both before and after the call to echart.resize(), and with finished and rendered, all with no luck.
setTimeout with 100ms, seems to render 30-40 ms after the finished event does, which is apparently enough. I also tried to just use setTimeout with 0 ms, and the microtask queue, but both seem to get called too soon
There was a problem hiding this comment.
Hmm, tricky. Does finished need to be called outside of handleResize?
I also noticed that resize has an animation option. Is it possible to utilize transitionend or animationend if the animation uses CSS?
|
Size Change: +313 B (+0.02%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
shaneeza
reviewed
Jan 24, 2025
| if (zoomSelect?.xAxis || zoomSelect?.yAxis) { | ||
| function enableZoomOnRender() { | ||
| echart.enableZoom(); | ||
| // Zooming triggers a render itself. This prevents an infinite loop. |
There was a problem hiding this comment.
You're saying that calling echart.enableZoom(); on every render causes an infinite loop unless you call echart?.off?
There was a problem hiding this comment.
I'm saying that enabling zoom triggers a render, so once we enable it, we want to remove the handler or else there will be an infinite loop of render -> enable -> render -> etc.
I've updated the comment to try to make it more clear. Let me know if it isn't!
…charts-resize-with-container
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
ResizeObserveron the chart container to trigger resize.🎟 Jira ticket: LG-4797
✅ Checklist
For bug fixes, new features & breaking changes
pnpm changesetand documented my changesFor new components
🧪 How to test changes