-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
fix for Scattergl animation bug #6452
base: master
Are you sure you want to change the base?
Conversation
If anyone has time to double check these changes it would be very helpful. Essentially, there are a few issues that this fix should address:
|
@@ -348,7 +347,7 @@ var exports = module.exports = function plot(gd, subplot, cdata) { | |||
(yaxis._rl || yaxis.range)[1] | |||
] | |||
}; | |||
var vpRange = Lib.repeat(vpRange0, scene.count); | |||
var vpRange = Lib.repeat(vpRange0, cdata.length); |
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.
Not sure about this one
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.
Is this change required?
Code to reproduce: https://jsfiddle.net/gcptmL6v/ With the proposed fix: https://jsfiddle.net/q2wt06d3/ |
@alexturcea great! Nicely done, and your fiddles are very convincing. I'll let @archmoj give the code a review, but we'll need to adapt your fiddle to a test we can run on CI - maybe in animate_test.js? And hopefully we can pare it down to something small and readable 😉 A bunch of failures in the CI runs, but they don't look like the kind of thing that would have resulted from your changes. |
Hi @alexcjohnson @archmoj , I synched this with the main branch, still hoping to have it merged 😄 |
Thanks @alexturcea - as I mentioned before, we'll need to adapt your fiddle into a test, would you be able to try that? We'll also need a draftlog item. |
Please fetch |
@@ -53,7 +53,6 @@ var exports = module.exports = function plot(gd, subplot, cdata) { | |||
return; | |||
} | |||
|
|||
var count = scene.count; |
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.
Would it be better to just var count = cdata.length
in that case - and remove all the other changes?
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.
Good point! @eiriklv Could you please fetch upstream/master
on your fork and test it by opening a PR?
@alexturcea Are you interested in completing this PR? |
closes #6251