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
Conengmo
left a comment
There was a problem hiding this comment.
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_colordoesn't get passed when another class is created, like inStepColormap.to_linear(). Please check you got them all. - The default argument to
text_colorshouldn't be an empty string (see comment). - The
text_colorargument 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
| vmin=0.0, | ||
| vmax=1.0, | ||
| caption="", | ||
| text_color="", |
There was a problem hiding this comment.
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. |
Conengmo
left a comment
There was a problem hiding this comment.
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 :) |
* Add ability to specify text_color for ColorMap. * Modify colormap example to include text color. * Make text_color comment clearer. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Return executionCount to 1 * Use implied line contrinuation to shorten lines * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Apply black autoformatting. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * ENH: add ability to change color of legend's text * Minor fixes for new text_color argument * add default value to docstring --------- Co-authored-by: ThomasBur <thomasbur@outlook> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Frank <33519926+Conengmo@users.noreply.github.com>
Addresses #130 by adding a
text_coloroptional parameter to theColorMapclass which changes thefill:attribute of the caption and tick text.