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

Minor changes to animation code ahead of the array animation fix #2313

Merged
merged 7 commits into from Feb 5, 2018

Conversation

Projects
None yet
2 participants
@monfera
Copy link
Contributor

commented Jan 31, 2018

These should involve no actual logic change, see the commit texts.

@monfera monfera self-assigned this Jan 31, 2018

@monfera monfera force-pushed the array-anim branch from 54c6f3c to 6efb0a7 Jan 31, 2018

@etpinard
Copy link
Member

left a comment

Thanks for the PR! I'm not sure I understand your first commit. Looking for a better explanation.

'" with a frame whose name of type "number" also equates to "' +
name + '". This is valid but may potentially lead to unexpected ' +
'behavior since all plotly.js frame names are stored internally ' +
'as strings.');

if(numericNameWarningCount > 5) {
Lib.warn('addFrames: This API call has yielded too many warnings. ' +
if(numericNameWarningCount === numericNameWarningCountLimit) {

This comment has been minimized.

Copy link
@etpinard

etpinard Feb 1, 2018

Member

Why === instead of > ?

This comment has been minimized.

Copy link
@monfera

monfera Feb 1, 2018

Author Contributor

Explanation of 1st commit: it looked to me as though Lib.warn continued to be invoked past the 5 warnings, because I didn't see an actual condition that'd bypass this code path, so I added an extra condition in the if line. With the commit, if numericNameWarningCount >= numericNameWarningCountLimit then it doesn't descend in the if, so it won't warn there anymore. The condition for raising the "Too many warnings" is an equality check, because numericNameWarningCount is initially 0 but gets incremented past the condition check ie. before this check. If numericNameWarningCount is 5 here, then we've been through this conditional branch 5 times (with numericNameWarningCount values 1, 2, 3, 4, 5); we emit the "too many warning" things and won't come into this conditional branch again. Hope I got it right but maybe you're seeing an off-by-one error?

This comment has been minimized.

Copy link
@etpinard

etpinard Feb 1, 2018

Member

Ok. That makes sense. Can we add a test for this? Using jasmine spyOn(Lib, 'warn') should make this task relatively easy.

var thisUpdate = {};

for(var ai in data[i]) {
thisUpdate[ai] = [data[i][ai]];

This comment has been minimized.

Copy link
@etpinard

etpinard Feb 1, 2018

Member

Strange. I wonder what Ricky had in mind here. Good catch 👌

This comment has been minimized.

Copy link
@monfera

monfera Feb 1, 2018

Author Contributor

Could've been just some initial logic that remained there.

.then(function() {
// Five (`var numericNameWarningCountLimit = 5`) warnings and one warning saying that there won't be more warnings
expect(Lib.warn.calls.count()).toEqual(5 + 1);
}).catch(fail).then(done);

This comment has been minimized.

Copy link
@etpinard

etpinard Feb 5, 2018

Member

Excellent test. Thanks!

@etpinard

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

Good stuff. Thanks for taking on the animation code.

💃

@monfera monfera merged commit d30a3bd into master Feb 5, 2018

4 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details

@monfera monfera deleted the array-anim branch Feb 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.