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

Fix autoResize functionality #271

Merged
merged 3 commits into from
Nov 7, 2020
Merged

Conversation

ThomasBower
Copy link
Contributor

@ThomasBower ThomasBower commented Oct 31, 2020

Resolves #60, resolves #141


Personally, I believe that the current autoResize implementation is incorrect. Currently, the resize() function on the echarts instance is only called if the window resize event is fired. If I set autoResize to true, I would expect the graph to resize whenever its container size is changed, not only when the window is resized.

Moreover, even when the window is resized, there is no guarantee that the chart container needs to be resized, causing needless calls to the resize function. What we should actually be doing is checking when the surrounding container is resized as this is a much more reliable indicator that we need to resize and redraw the graph.

@xieziyu has previously justified this behaviour in #60 by saying (roughly translated) "the size change of the container is usually caused by some human factor. That is to say, the code should know when the container change will occur.", but I don't think it is reasonable to expect every other component on the page which could potentially cause the graph container to change size to directly call the resize event on the echarts instance. This behaviour should be handled in the chart component directly.

Please let me know what you think, as I think the behaviour shown here is a big improvement over the current implementation.

2020-10-31 21 41 22-min

@xieziyu
Copy link
Owner

xieziyu commented Nov 1, 2020

Appreciate for your work. I agree with you about the expected behaviour.
But the ResizeObserver spec is still in the ED status, and not supported by some browers. Maybe a polyfill should be imported when needed.

@ThomasBower
Copy link
Contributor Author

ThomasBower commented Nov 1, 2020

@xieziyu Good point and thanks for the feedback. It has ~86% coverage but some big gaps on some mobile browsers and Internet Explorer. I've imported the polyfill we can use.

@ThomasBower
Copy link
Contributor Author

@xieziyu Did you get a chance to have a look at the changes? Thanks.

@xieziyu xieziyu merged commit 714861e into xieziyu:master Nov 7, 2020
@ThomasBower ThomasBower deleted the fix-resizing branch November 7, 2020 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants