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

ColorMap text color #160

Merged
merged 13 commits into from
May 7, 2024
Merged

Conversation

ThomasBur
Copy link
Contributor

Addresses #130 by adding a text_color optional parameter to the ColorMap class which changes the fill: attribute of the caption and tick text.

Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

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

Thanks @ThomasBur for your contribution on this one! I think you did well, but we're not fully there yet.

  • In some places self.text_color doesn't get passed when another class is created, like in StepColormap.to_linear(). Please check you got them all.
  • The default argument to text_color shouldn't be an empty string (see comment).
  • The text_color argument is not included in every docstring.

Alternatively, as an idea, you could opt to not make it a class argument (so not include it in the __init__ call. But do set self.text_color = "black". That way users can overwrite it if they want, like in your example: colormap.text_color = 'cyan'. But we don't have to pass it around.

  • The color is applied in _repr_html_, meaning it is applied when rendering Jupyter Notebooks. We still need to also apply it for the 'regular' html case. For that, look at Colormap.render(), which renders the html, and templates/color_scale.js, which is the Javascript template we render into. Now I'm not familiar with d3, so I can't really say how the color should be applied here.

Hope you can take a look at these comments, and figure out how to apply the color in color_scale.js as well. Happy to review this one again!

@@ -241,13 +261,15 @@ def __init__(
vmin=0.0,
vmax=1.0,
caption="",
text_color="",
Copy link
Member

Choose a reason for hiding this comment

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

If you set the default argument here to an empty string, it will overwrite the default "black". I suggest you use "black" as the default everywhere.

@ThomasBur ThomasBur requested a review from Conengmo April 5, 2024 05:09
@ThomasBur
Copy link
Contributor Author

Hi @Conengmo thank you for the feedback.
I worked out that all the places I missed for passing and docstrings where were caption was also missed. Fixed it for both of those in al spots.
I got render to work, and have also tested it with folium and geopandas.
Happy to make any other changes as required :)

Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

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

Looks good! I made a slight change to the docstrings to include the default argument value. Ready to merge!

@Conengmo Conengmo merged commit 8ed9839 into python-visualization:main May 7, 2024
9 checks passed
@Conengmo
Copy link
Member

Conengmo commented May 7, 2024

Thanks Thomas for working on this one, much appreciated

@shriv
Copy link

shriv commented Oct 16, 2024

Hiya @Conengmo and @ThomasBur. This is exactly what I need but I can't figure out how to add text_color inside gpd.explore(). Also, which version of geopandas is this shipped with? I'm currently running 0.14.4 which seems to be connected with branca 0.7.1.

@Conengmo
Copy link
Member

Hi @shriv , this change was included in the Branca 0.8.0 version. Looking at the geopandas repo, it doesn't seem the Folium/Branca version is pinned, so you may just update Branca yourself: pip install branca --upgrade. Hope that helps!

@shriv
Copy link

shriv commented Oct 17, 2024

Awesome! Thank you @Conengmo :-) I will update branca to get the changes 👍 Do you also have a snippet of how I can include the text_color attribute within gpd.explore()?

@Conengmo
Copy link
Member

Do you also have a snippet of how I can include the text_color attribute within gpd.explore()?

Not really, I'd have to look into that as well. Good luck :)

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