Skip to content

scattergl visible restyle fix - #2442

Merged
etpinard merged 6 commits into
masterfrom
scattergl-visible-fix
Mar 6, 2018
Merged

scattergl visible restyle fix#2442
etpinard merged 6 commits into
masterfrom
scattergl-visible-fix

Conversation

@etpinard

Copy link
Copy Markdown
Contributor

fixes#2441 and 🌴 a few things in scattergl/index.js along the way.

cc @dfcreative@alexcjohnson

- refs to regl2d objects are kept even when options objects are emptied
e.g. in the case of legend visible toggles, so don't try to redraw
regl2d object with old options.
- use same convert routine for x and y errors,
- use same reset options object when initializing and resetting
scene options.
spyOn(scene.error2d, 'draw');
spyOn(scene.scatter2d, 'draw');

return Plotly.restyle(gd, 'visible', 'legendonly', [0]);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On master, this gives:

peek 2018-03-05 17-29

if(scene.fill2d) scene.fill2d.draw(i);
if(scene.fill2d && scene.fillOptions[i]) {
// must do all fills first
scene.fill2d.draw(i);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear improvement over master - but this does raise the question of whether this (not your change, but the structure surrounding it) is really the right behavior, particularly with fill: 'tonext' but also with multiple fills to zero. I can try this out tomorrow and see if my interpretation of what this is doing is accurate, but In svg scatter, each fill gets drawn below any trace that it's attached to (even the trace before it if it's a fill: 'tonext') but above any other trace that comes before it. If all the lines and points are above all the fills, you can get points and lines that look disconnected from their own fills.

Comment threadsrc/traces/scattergl/attributes.js Outdated
opacity: extendFlat({}, plotAttrs.opacity, {
editType: 'calc'
}),
opacity: extendFlat({}, plotAttrs.opacity)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't even need the extendFlat, right? overrideAll will take care of that.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 4776832

markerOptions.positions = positions;
}

function makeErrorOptions(axLetter, errorOpts, vals) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

scene = subplot._scene = Lib.extendFlat({}, reset, first);

// apply new option to all regl components (used on drag)
scene.update = function update(opt) {

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @dfcreative you were right! scene.update is still used on drag. Added spy+assertion below to 🔏 this thing down.

@etpinard

Copy link
Copy Markdown
ContributorAuthor

Dima verbally ✅ this PR to me.

@alexcjohnson anything else? Your comment in #2442 (comment) probably deserved an issue of its own.

@alexcjohnson

Copy link
Copy Markdown
Collaborator

💃
I'll look at the layering issue and make an issue if it's off.

@etpinard
etpinard merged commit 069a6f0 into masterMar 6, 2018
@etpinard
etpinard deleted the scattergl-visible-fix branch March 6, 2018 23:37
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugsomething broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scattergl 'lines+markers' traces don't toggle properly

3 participants

@etpinard@alexcjohnson@dy