Proper parcoords axis tick scale when ticktext is unordered
#1945
Conversation
monfera
force-pushed
the
parcoords-unordered-ticktext
branch
from
dc1f191 to
926e7c6
Compare
src/traces/parcoords/parcoords.js
Outdated
| @@ -557,7 +557,7 @@ module.exports = function(root, svg, styledData, layout, callbacks) { | |||
| .outerTickSize(2) | |||
| .ticks(wantedTickCount, d.tickFormat) // works for continuous scales only... | |||
| .tickValues(d.ordinal ? // and this works for ordinal scales | |||
| sdom.map(function(d, i) {return texts && texts[i] || d;}) : | |||
| sdom.map(function(d, i) {return texts && texts[i] !== undefined ? texts[i] : d;}) : | |||
There was a problem hiding this comment.
Maybe filter out null too?
Actually, looks like cartesian tickvals / ticktext just stringifies falsy values: https://codepen.io/etpinard/pen/aywJNK?editors=0010 - which might be considered a bug, but I'd vote for making the parcoord implementation consistent(ly buggy).
There was a problem hiding this comment.
What this does right now: if ticktext[i] is undefined, ie. the array is nonexistent or too short, then it'll use the tickvals[i] value (which can be the intention), represented as the d in the green line. If it's however defined, then it'll be used (and stringified) whether falsy or not, in line with what you describe for Cartesian. Is it OK or would you rather that a missing ticktext array or missing ticktext[i] yields an 'undefined' as the tick label?
There was a problem hiding this comment.
Is it OK or would you rather that a missing ticktext array or missing ticktext[i] yields an 'undefined' as the tick label?
Ideally, it would be nice to make parcoords reuse some of the cartesian/axes.js routines.
Now, should we consider the current cartesian behavior a bug? I'd vote yes. I'd think only typeof strings ticktext values should be visible cc @alexcjohnson
There was a problem hiding this comment.
What should be the next step? It'd be good to see the missing tick label fixed promptly and continuing the above convo here, or I could copy it in a separate issue to be discussed.
There was a problem hiding this comment.
What should be the next step?
Enjoy your weekend. We'll revisit this Monday 🎉
There was a problem hiding this comment.
@alexcjohnson to summarise if I understand your latest comments and Étienne's comment on the top, this PR needs these:
- in
parcoords, ifticktext[i]is eitherundefinedornull, then we consider it unsupplied, ie. revert to just usingtickvals[i](the current PR currently checks forundefinedonly but I can extend) tickvals[i]should be subject totickformatas on my above screenshot with the percentages in column 3 and 4.- these things should be locked down in test so that, when we get around to axis unification, we can detect regressions
- I'd create an issue that refers to comments in this PR that would be the basis for considering a change in cartesian such that it also does what's described in point 1 of this list
There was a problem hiding this comment.
@monfera that all sounds exactly right 💯
There was a problem hiding this comment.
in parcoords, if ticktext[i] is either undefined or null, then we consider it unsupplied, ie. revert to just using tickvals[i] (the current PR currently checks for undefined only but I can extend)
Maybe even: if ticktext[i] is either undefined or null or false [...]?
There was a problem hiding this comment.
I'd let false pass through (ie get stringified) - not super useful, but it would be weird to stringify true but turn false into some number. null and undefined don't have opposites in the same way.
There was a problem hiding this comment.
Thanks for the clarifications! The commit is done but I want to do some code simplification once crossfilter is tightened up. I can push where I am though.
|
The last commit unifies the discussed logic, please take another look at it 👍 |
src/traces/parcoords/parcoords.js
Outdated
| return dd; | ||
| } | ||
| }) : | ||
| sdom.map(toText(Lib.identity), texts) : |
There was a problem hiding this comment.
Am I missing someting or are the () not in the right place here?
sdom.map(toText(Lib.identity, texts))
There was a problem hiding this comment.
Thanks @alexcjohnson for the catch, thought it was magic that it still worked! Turns out, the domainScale change (same PR, higher up in the code) made scale.domain() to return the desired values anyway, which got mapped with the identity function (as texts was not supplied ie. falsey).
There was a problem hiding this comment.
:) surprised me too, esp. since this seems to be fully tested. Thanks for figuring out what's really happening.
|
Looks great to me! 💃 |
Fixes #1941 whose original report is plotly/plotly.R#1096 (comment).
The problem arose due to not properly supporting unordered tick labels (
ticktext), fixed with this commit, line 72.While at it, an additional safeguard was added in line 560, for the eventuality that a falsey
ticktexteg.0is used such that the correspondingtickvalsvalue is non-zero.A test change was made to exercise both the out of order
ticktextand the falsey value (0). It is expected that the0tick label does show up; and that it shows up as zero rather than its correspondingtickvalsvalue which is 3. Note: the purpose oftickvalsis the appropriate vertical placement of the tick to give the user flexibility, for example, uneven distance between adjacent ticks.