Conversation
|
@archmoj have you noticed any broken behaviour that can be traced back to not having positive sizes in the calc step? |
|
I'm thinking if might be safer to move some of these checks to |
I would say it is better if one checks the inputs as early as possible which is in this case the |
Ok nice! Now, what about the other
are they showing bugs currently on master? I'm just trying to make sure we don't sanitize things twice. |
src/lib/index.js
Outdated
| return lib.mergeArray(traceAttr, cd, cdAttr, function(v) { | ||
| var w = +v; | ||
| return w > 0 ? w : 0; | ||
| return isNaN(w) ? NaN : w > 0 ? w : 0; |
There was a problem hiding this comment.
I think you want isNumeric here. Have tried something like
'marker.line.width': [NaN, null, undefined, [], {}, 0]`?
There was a problem hiding this comment.
Works OK with those cases. Using isNumeric might be slow.
There was a problem hiding this comment.
Have you tried running benchmarks here? I doubt that isNumeric is that much slower than isNaN
There was a problem hiding this comment.
Moreover, could we lock those cases down in a _module.calc test?
Also regarding funnelarea |
test/jasmine/tests/bar_test.js
Outdated
| }], {}); | ||
|
|
||
| var cd = gd.calcdata; | ||
| assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); |
There was a problem hiding this comment.
Does having NaN in calcdata[i][j].mlw lead to console errors?
There was a problem hiding this comment.
console errors e.g. when drawing a svg path?
There was a problem hiding this comment.
I didn't notice any:
Scatter codepen
Bar codepen
There was a problem hiding this comment.
But that's a very good question.
Now I am wondering if we should patch this test instead of the lib.mergeArrayCastPositive to accept 0 instead of NaN?
There was a problem hiding this comment.
I didn't notice any:
Ok, but looks like marker pts with mlw = NaN render as if they had mlw = 0, so I would prefer making NaN cast to 0 during calc.
There was a problem hiding this comment.
Good call. Done in f9db523 and fixed scattegeo test.
|
Nicely done @archmoj - thanks very much!! |
|
@etpinard may I merge this PR? |
|
Oops - I forgot the 💃 |
|
Yes, please merge this PR - thanks again! |
2 participants
Follow up of #4056 &
Fixes #4057.
Fix and apply
Lib.mergeArrayCastPositiveduringcalcstep of various traces to ensure having positive inputs for size attributes.@plotly/plotly_js