Colorspec doc corrections and clarifications #625
Conversation
(cherry picked from commit 4d0b2ff)
| @@ -64,14 +69,16 @@ module.exports = function makeColorScaleAttributes(context) { | |||
| autocolorscale: extendDeep({}, colorScaleAttributes.autocolorscale, { | |||
There was a problem hiding this comment.
@etpinard Is it still true that both or neither of cmin,cmax must be set? My experience with scatter3d showed that omitting one of them did the reasonable thing: it behaved as if the omitted cm... value were set to the lowest (or highest) value of the color numeric array.
There was a problem hiding this comment.
Correct. It is a little more subtle at the moment. See this line in the colorscale defaults.
If one of cmin or cmax isn't valid, then autoscale is defaulted to false. But, if the user set autoscale to true, the missing cmin or cmax gets auto-calc'ed.
There was a problem hiding this comment.
@etpinard yes, tests support you, assuming you actually meant cauto (autocolorscale just toggles between a default palette vs. a palette determined by the user-supplied colorscale).
In any case, our common conclusion is that the user does not need to set both of cmin and cmax, yet the current doc says both are needed. I'll leave it as it is because, with the current helptext, partial cmin / cmax specification leads to undefined results, which is of course fine even if we handle it gracefully. I just wasn't sure if you wanted to keep this constraint in the helpstring, or it was a case of just not having updated the doc string.
There was a problem hiding this comment.
p.s. by the way, a reason for even considering partial cmin/cmax is that there's utility in clamping only one side of the domain (e.g. use the scale for water temperatures but all frozen, sub-zero points should be icy bue), which I presume is the reason for having it implemented resiliently against the case of partial bounds specification.
monfera
changed the title
| ' it controls whether the first/last colors in `colorscale` correspond to', | ||
| ' the lowest/highest values in `color` (`cauto: true`), or the `cmin`/`cmax`', | ||
| ' values (`cauto: false`).', | ||
| ' Defaults to `false` when `cmin`, `cmax` are set by the user.' |
There was a problem hiding this comment.
This is just clarification as the former text is explanatory only once the user is already familiar with it.
There was a problem hiding this comment.
I think the wording in this is still a bit muddy - the In this case sentence in particular.
Can we not say (in some way) the cmin and cmax "map" the colorscale bounds to data or something?
There was a problem hiding this comment.
@mdtusz I've updated it to
In this case, it controls whether the color range represented by `colorscale` is mapped to
the value bounds of the `color` array (`cauto: true`), or to the `cmin`..`cmax`
value bounds (`cauto: false`).
Is it okay? I feel there's a tradeoff among using the best terminology (concept of mapping), comprehension by all ('what is mapping?') and brevity; maybe this version is at a better point in that space. (I haven't pushed it yet to avoid unnecessary CI testing)
There was a problem hiding this comment.
Summoning Docs-Master @cldougl!
There was a problem hiding this comment.
taking a 👀 now -- give me 1 min to catch up on the context
| @@ -19,7 +19,7 @@ module.exports = function makeColorScaleAttributes(context) { | |||
| role: 'style', | |||
| description: [ | |||
| 'Sets the ', context, ' color. It accepts either a specific color', | |||
| ' or an array of values that are mapped to the colorscale', | |||
| ' or an array of numbers that are mapped to the colorscale', | |||
There was a problem hiding this comment.
Numbers or numeric strings to be more precise. Not sure if it's worth adding that amount of details in here though.
There was a problem hiding this comment.
@etpinard I can cancel the PR and just make this single word change in the main PR. However to understand my motive, I started some work on the basis of what these docs say, and while their meanings are trivial to you due to developing these props in the first place, it was partly opaque to me, and in one case, misleading, so I had to resort to trial / error (at that point I intentionally didn't just look at the code). So I thought that if I as a developer had a problem understanding it then others may benefit from a modest rephrasing. So I'm glad to do what's the best fit for the system overall. Unrelated: I didn't quite understand the numeric string part as in JSON you'd pass on an array of plain numbers.
| ' Reverses the colorscale.' | ||
| ' Reverses the color mapping if true (`cmin` or lowest `color` value will', | ||
| ' correspond to the last color and `cmax` or highest `color` value will', | ||
| ' correspond to the first color).' |
There was a problem hiding this comment.
I like the phrase color mapping > colorscale.
I like this additional info but Idk if 'lowest color // highest color' are necessary (min/max are pretty universally clear imo). I would probably go with:
' Reverses the color mapping if true (cmin will correspond to the last color in the array and cmax will correspond to the first color).'
|
General comment for backburner type thinking: while currently the |
(cherry picked from commit a161fa7)
|
Attribute updates done in #617 |
(cherry picked from commit 4d0b2ff)
This is a pure document update PR that fixes some docs and improves on some others. Should I be wrong on some of them, it helps clarify.
I also tried to use more of the tangible, everyday words and fewer such terms that are clearer to some but perceived as vague by those who e.g. aren't familiar with the domain / range terms of functions (scales).