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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix scatter fill regression #3168

Merged
merged 2 commits into from Oct 26, 2018

Conversation

Projects
None yet
2 participants
@etpinard
Copy link
Member

commented Oct 26, 2018

This PR fixes an unreleased regression originating from #3087. I'd like to get this in before the 1.42.0 release.

Scatter fills on master currently do not restyle correctly and lead to these logs:

image

but no failed tests. That's because those logs come from:

try {
shape.call(Color.fill, d[0].trace.fillcolor);
}
catch(e) {
Lib.error(e, s);
shape.remove();
}

which do not make failTest throw, leading to false-positive tests.


The first commit fixes and 馃敀s the bug, and the second commit 馃敧s that try-catch (which is at least 4 years old) which is probably a bit more dangerous, but I'm willing to take that chance. I hope @plotly/plotly_js agrees.

etpinard added some commits Oct 26, 2018

fix scatter fill restyle regression
... by setting trace._ownfill on every linkTraces call,
    so we don't let relinkPrivateKeys mess things up.

    Previously, the scatter fill restyle test did not fail,
    but did log a error as Drawing.fillGroupStyle logs
    in a try-catch when things go wrong instead of errorring out.
@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

Good catch! I noticed those logs but studiously ignored them 馃檲 - glad someone is paying more attention!!

The first commit fixes and 馃敀s the bug

How does this 馃敀 it?

the second commit 馃敧s that try-catch ... which is probably a bit more dangerous, but I'm willing to take that chance.

Me too. We have enough tests at this point, don't worry about removing it. As evidenced here, a try/catch like this is more likely to paper over a problem than to provide useful behavior!

@etpinard

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

How does this it?

By spying on Lib.error.

@etpinard

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

... by I removed that spy in the second commit 1f3c10c as now the fillGroupStyle errors lead to failTest.

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

Got it, thanks! 馃拑

@etpinard etpinard merged commit 15afd7b into master Oct 26, 2018

7 checks passed

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

@etpinard etpinard deleted the fix-scatter-fill-regression branch Oct 26, 2018

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