Colorscales

[simleb]

I added transformed continuous color scales to fix #744 and fix what I think is a typo in asinh_formatter.

It seems to work fine but I am a Gadfly beginner so please make sure I didn't break anything!

[simleb]

I'm not sure I understand what made the CI build fail but I'm happy to try fix it if you guide me.

[darwindarak]

I think the problem is on this line. Since the trans argument did not exist before, and a default value is not provided, it is breaking any existing code that simply calls ContinuousColorScale(f). Try turning it to trans::ContinuousScaleTransform = identity_transform?

[simleb]

That worked, thanks @darwindarak !

[timholy]

LGTM. But I suspect this could use a test, otherwise your shiny new functionality might get broken by some other well-meaning future contribution 😄.

The general guideline seems to be to add a new file to the test/ folder, with only one rendered display per file. Others can correct me if I'm wrong.

[simleb]

I added a single test instead of testing all variants since that seems to be the norm. (I did test them all manually.) I also updated the docs.

There is one more thing I'd like to do but I'd rather ask first: update the docs such that the name color_continuous takes precedence over color_continuous_gradient, for consistency with [xy]_continuous. We could even go one step further and deprecate color_continuous_gradient. Let me know what you think.

[darwindarak]

I think we should move towards making test-all-variants the norm if possible. Why not?

It seems a little awkward to group all the variants under the docs for continuous_color. It might make more sense to show them as convenience functions for ContinuousColorScale. What do you think?

[simleb]

@darwindarak Sure I can add more exhaustive tests, I just didn't want to add too many files.

About the docs, yes and no. I agree that color_continuous might not the best place since they are not arguments but actually alternatives to color_continuous. I did this to mimic the existing doc for x_continuous. However I find it pretty discoverable this way since they are indeed continuous color scales, they are just transformed. And it makes the index of the docs less cluttered than having all variants listed.

Also, it looks very odd to me that ContinuousColorScale is the only camel-cased identifier in the list. It's pretty inconsistent. In my opinion, color_continuous should accept an optional argument corresponding to the f of ContinuousColorScale (renamed to something more meaningful like color_mapping or colormap) and we should remove ContinuousColorScale from the docs. But that should probably be another PR.

[darwindarak]

That makes sense, so group everything under color_continuous, and deal with ContinuousColorScale later?

[simleb]

Sounds good. It's ready to merge then. In a different PR, I'll rename color_continuous_gradient in the docs to color_continuous, update color_continuous to accept ContinuousColorScale's function argument, and remove ContinuousColorScale from the docs. We can discuss if color_continuous_gradient should be deprecated or not in the new PR.