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 scatter3d opacity restyle bug #5958

Merged
merged 4 commits into from
Oct 4, 2021

Conversation

dwoznicki
Copy link
Contributor

This is a fix for #5957. I've modified the scatter3d attributes to make opacity trigger a full layout replot when the opacity is changed via Plotly.restyle. Note that this solution is derived from the scattergl trace (https://github.com/plotly/plotly.js/blob/master/src/traces/scattergl/attributes.js), which handles opacity changes correctly.

… scatter3d trace was not updating the graph
@archmoj archmoj added the community community contribution label Sep 30, 2021
@archmoj
Copy link
Contributor

archmoj commented Sep 30, 2021

Thanks for the PR.
It appears the schema needs an update.
Please run the following after npm start.

npm run schema && \ 
git add test/plot-schema.json && \
git commit -m "update plot-schema diff"

@archmoj archmoj added the bug something broken label Sep 30, 2021
@archmoj
Copy link
Contributor

archmoj commented Oct 4, 2021

The fix looks great.

  1. Please add a test in the interaction section of test/jasmine/tests/scatter3d_test.js for example
Plotly.newPlot(
  gd,
  [
    {
      type: "scatter3d",
      x: [0],
      y: [0],
      z: [0],
      mode: "markers",
    }
  ],
).then(function() {
  Plotly.restyle(gd, 'opacity', 0.5).then(function() {
    expect(gd._fullLayout.scene._scene.glplot.objects[0].hasAlpha).toEqual(true);
  });
});

Also please create draftlog/5958_fix.md file as described here for example

 - Fix scatter3d opacity restyle bug [[#5958](https://github.com/plotly/plotly.js/pull/5958)],
   with thanks to @dwoznicki for the contribution!

@archmoj
Copy link
Contributor

archmoj commented Oct 4, 2021

Nicely done.
💃

@archmoj archmoj merged commit 6e4db26 into plotly:master Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken community community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants