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 to_rgb_color_list when passing rgba colors #3478

Merged
merged 4 commits into from
Dec 20, 2021
Merged

Fix to_rgb_color_list when passing rgba colors #3478

merged 4 commits into from
Dec 20, 2021

Conversation

janosh
Copy link
Contributor

@janosh janosh commented Nov 18, 2021

Closes #3477.

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or modified existing tests.
    No tests for to_rgb_color_list at all yet but not necessary as dependent function create_annotated_heatmap is about to be deprecated (see below).
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.

@janosh
Copy link
Contributor Author

janosh commented Nov 18, 2021

Another probable issue is that to_rgb_color_list does not handle hsl(a) colors at all, even though create_annotated_heatmap does. Should we add a case for that?

@janosh
Copy link
Contributor Author

janosh commented Nov 24, 2021

I have added a CHANGELOG entry if fixing/changing/adding anything substantial.

Does this require a changelog entry?

@nicolaskruchten
Copy link
Contributor

Hi, and thanks for your pull request! Yes, this probably needs a changelog entry.

I should note that in the next release of this library, we will be marking create_annotated_heatmap as deprecated, as px.imshow() will be able to add text to heatmaps natively, and all the colour-handling will happen in the Javascript layer so as to be more robust.

@janosh
Copy link
Contributor Author

janosh commented Nov 24, 2021

Good to know. So does px.imshow on current master already handle text? If so I might switch right away.

@janosh
Copy link
Contributor Author

janosh commented Dec 14, 2021

@nicolaskruchten This is ready for review.

@nicolaskruchten
Copy link
Contributor

Thanks @janosh ! Yes, I've just merged the new changes to px.imshow to master, and I've marked create_annotated_heatmap as deprecated, so I'd rather not make any new modifications to it now, if that's OK.

@janosh
Copy link
Contributor Author

janosh commented Dec 20, 2021

It's still a fix and would allow me to release some code that relies on this. Merging fixes even to deprecated code makes sense imo.

elif "#" in color_str:
color_str = color_str.strip()
if color_str.startswith("rgb"):
return [int(v) for v in color_str.strip("rgba()").split(",")]
Copy link
Contributor

Choose a reason for hiding this comment

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

does this now still work with rbg(...) and rgba(...) both ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does. Tested it. As I mentioned, doesn't handle HSL(A) yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. After this next release I won't be accepting any more PRs to this deprecated code so let's not work on any HSL stuff please :)

@nicolaskruchten
Copy link
Contributor

OK, thanks!

@nicolaskruchten nicolaskruchten merged commit 35cbe11 into plotly:master Dec 20, 2021
@janosh
Copy link
Contributor Author

janosh commented Dec 20, 2021

Thanks for merging!

@janosh
Copy link
Contributor Author

janosh commented Jan 1, 2022

I should note that in the next release of this library, we will be marking create_annotated_heatmap as deprecated, as px.imshow() will be able to add text to heatmaps natively, and all the colour-handling will happen in the Javascript layer so as to be more robust.

px.imshow doesn't seem to provide a way to set xgap/ygap. Also would be cool if the https://plotly.com/python/imshow examples included one with pandas where a df with a text column is used to render hover tooltips. Doesn't feel like imshow is ready to replace create_annotated_heatmap yet.

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.

create_annotated_heatmap throws on rgba in colorscale
2 participants