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

Scattergl improvements for: fill tozero with bad values fix, fills layering fix, add some line.shape values #3087

Merged
merged 27 commits into from
Oct 11, 2018

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Oct 9, 2018

This PR builds up on @ErwanMAS's #2979 where
I added a few commits to make his fix more in line with some scatter logic.

In brief, this PR:

  • has a fix for scattergl fill 'tozero' with bad (e.g. undefined) values
  • multiple scattergl fill ordering fixes
  • add support for line.shape values 'hv', 'vh', 'hvh', 'vhv' (only missing 'spline')

Set to be part of v1.42.0, cc @plotly/plotly_js

ErwanMAS and others added 25 commits September 8, 2018 19:41
 - when undefined values with option fill tozerox or tozeroy
 - order of fill scattergl when overlap
still missing spline from scatter
 - new test with more overlap with more kind of objects
modified baseline for gl2d_fill_trace_tozero_order / gl2d_scatter_fill_self_next
add somes text traces into the baseline test
…/baselines/gl2d_scatter_fill_self_next.png )

- add a new test with 2 draw side by side
... by using _nexttrace and _prevtrace like SVG scatter does
@etpinard etpinard mentioned this pull request Oct 9, 2018
21 tasks
@alexcjohnson
Copy link
Collaborator

@ErwanMAS great work here. I'm really impressed by the new mocks, especially putting SVG and GL side-by-side, that helps a lot since it's otherwise not always obvious without poring over the data whether the filling & ordering is really supposed to be as it appears in the baseline image or not.

@etpinard beautiful simplifications in the last couple of commits. The mocks look fairly complete, so unless my question #3087 (comment) points to some otherwise untested case this should be ready to go!

@alexcjohnson
Copy link
Collaborator

Love it. Thanks for the quick turnaround @ErwanMAS @etpinard !
💃

@etpinard
Copy link
Contributor Author

💯 thank yous to @ErwanMAS , amazing work 🥇

@etpinard etpinard merged commit 5a24245 into master Oct 11, 2018
@etpinard etpinard deleted the etienne-scattergl-ordering branch October 11, 2018 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants