Skip to content

propagate WebGL traces with marker & line color arrays to hover labels #4867

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.

Sign up for GitHub

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

Jump to bottom
Merged
archmoj merged 7 commits into from
Sep 16, 2020
Merged

propagate WebGL traces with marker & line color arrays to hover labels #4867

archmoj merged 7 commits into from
Sep 16, 2020

Conversation

@archmoj
Copy link
Contributor

@archmoj archmoj commented May 25, 2020 •
edited
Loading

Addressing #2953 | demo.

This PR is a fix for displaying point colors set by scatter3d (marker|line).color with/without colorscale.
It also fixes display of point colors used in scattergl (marker|line).color array (i.e. without colorscale).

TODO:

  • add a jasmine tests

@plotly/plotly_js

@nasreddineskandrani
Copy link

nasreddineskandrani commented Aug 25, 2020 •
edited
Loading

hi,
@archmoj what is the state of this PR?
note that i am not able to see the chart in your demo link. Is it still working your side in the codepen?

@archmoj
Copy link
Contributor Author

archmoj commented Sep 3, 2020

hi,
@archmoj what is the state of this PR?
note that i am not able to see the chart in your demo link. Is it still working your side in the codepen?

@nasreddineskandrani
We basically need to add tests.
Also FYI - I updated the demo with a fresh build.

@archmoj archmoj requested a review from alexcjohnson September 3, 2020 20:12
alexcjohnson
alexcjohnson reviewed Sep 3, 2020
View reviewed changes
src/traces/scatter3d/convert.js Outdated

function arrayToColor(colors) {
if(!Lib.isArrayOrTypedArray(colors)) {
return _arrayToColor(colors);
Copy link
Collaborator

@alexcjohnson alexcjohnson Sep 3, 2020

Choose a reason for hiding this comment

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

return null here and avoid the outer isArrayOrTypedArray test in _arrayToColor?

Copy link
Contributor Author

@archmoj archmoj Sep 15, 2020

Choose a reason for hiding this comment

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

Good call. Done in 33fd07d.

alexcjohnson
alexcjohnson reviewed Sep 3, 2020
View reviewed changes
@alexcjohnson
Copy link
Collaborator

alexcjohnson commented Sep 3, 2020

@archmoj looks good - just a test as you mention, then this should be ready!

nasreddineskandrani
nasreddineskandrani reviewed Sep 5, 2020
View reviewed changes
@nasreddineskandrani
Copy link

nasreddineskandrani commented Sep 11, 2020 •
edited
Loading

in the demo, the orange flat circle is not orange. (the green same thing)
image

@archmoj
Copy link
Contributor Author

archmoj commented Sep 11, 2020

in the demo, the orange flat circle is not orange. (the green same thing)
image

Yeah. That's the limitation of handling typed arrays in scattergl at the moment.

alexcjohnson
alexcjohnson approved these changes Sep 16, 2020
View reviewed changes
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Looks great! 💃

@archmoj archmoj merged commit 34c82d2 into master Sep 16, 2020
@archmoj archmoj deleted the gl-hover-color branch September 16, 2020 01:00
@archmoj archmoj added this to the v1.56.0 milestone Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcjohnson alexcjohnson alexcjohnson approved these changes

+1 more reviewer

@nasreddineskandrani nasreddineskandrani nasreddineskandrani left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug something broken

Projects

None yet

Milestone

v1.56.0

Development

Successfully merging this pull request may close these issues.

4 participants

@archmoj @nasreddineskandrani @alexcjohnson