Skip to content

Pie chart slices support transparent rgba() colors #63

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

Merged
merged 1 commit into from
Dec 3, 2015

Conversation

tlrdstd
Copy link
Contributor

@tlrdstd tlrdstd commented Dec 2, 2015

Plotly.js pie charts accept rgba() marker colors, but drop any opacity value present. This codepen and image demonstrate: http://codepen.io/tlrdstd/pen/adoKgp

image

Running the same demo again with the attached change yields a chart with one transparent rgba() slice, one rgba() slice at 100% opacity, and an rgb() slice:

image

@etpinard
Copy link
Contributor

etpinard commented Dec 3, 2015

@tlrdstd 🍻 to out first community PR in the drawing code!

@alexcjohnson will take care of the reviewing duties on this one.

I'm not sure if handling rgba opacity in the calc step is the most plotly.js-esque way of doing this. Maybe the plot step would be a better place for this.

@alexcjohnson
Copy link
Collaborator

@tlrdstd looks good, thanks!

@etpinard normally colors would get set in the style step (even later than plot), but for pies this is actually the correct step. The reason is that pie colors talk between traces for color matching labels across pies, so we need to assign them early in the process.

We need to include this in a test - perhaps give some opacity to one of the colors in pie_labels_colors_text? Looks like we don't really have instructions written up for updating test images yet, maybe for now I'll just merge this in and update the tests in a separate PR.

alexcjohnson added a commit that referenced this pull request Dec 3, 2015
Pie chart slices support transparent rgba() colors
@alexcjohnson alexcjohnson merged commit 92888af into plotly:master Dec 3, 2015
@tlrdstd
Copy link
Contributor Author

tlrdstd commented Dec 3, 2015

@alexcjohnson Glad to help! I tried briefly to figure out how to write a test for this change, but I'm unfamiliar with docker, and didn't stumble across the JSON file you linked to.

I suspect/hope I'll have other pull requests (maybe with tests!) as I keep working with this library. Thanks for open sourcing it!

@alexcjohnson
Copy link
Collaborator

@tlrdstd excellent, I'll work with @etpinard to make it easier to get these image tests running locally - and I'll ping you when I have a PR that tests this particular patch so you see how it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants