-
Notifications
You must be signed in to change notification settings - Fork 68
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
ColorMap text color #160
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this 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 inStepColormap.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 atColormap.render()
, which renders the html, andtemplates/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!
branca/colormap.py
Outdated
@@ -241,13 +261,15 @@ def __init__( | |||
vmin=0.0, | |||
vmax=1.0, | |||
caption="", | |||
text_color="", |
There was a problem hiding this comment.
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.
Hi @Conengmo thank you for the feedback. |
There was a problem hiding this 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!
Thanks Thomas for working on this one, much appreciated |
Hiya @Conengmo and @ThomasBur. This is exactly what I need but I can't figure out how to add |
Hi @shriv , this change was included in the Branca 0.8.0 version. Looking at the |
Awesome! Thank you @Conengmo :-) I will update |
Not really, I'd have to look into that as well. Good luck :) |
Addresses #130 by adding a
text_color
optional parameter to theColorMap
class which changes thefill:
attribute of the caption and tick text.