Fix number named frames #1236
Fix number named frames #1236
Conversation
|
cc: @Kully re: problems with number-named frames This PR should hopefully fix the issues. It will always be slightly sketchy since it looks up frames by name and exact numeric equality is only a good idea with integers (JS sorta doesn't distinguish number types), but it is what it is. It will issue a warning if See: example: https://rreusser.github.io/demos/plotly-unsupported/numbered-frames-bugfix.html for an example that mixes strings and numbers with components. |
| 'behavior since all plotly.js frame names are stored internally ' + | ||
| 'as strings.'); | ||
|
|
||
| if(numericNameWarningCount > 5) { |
There was a problem hiding this comment.
Yeah, I published a module once where I said "meh, probably won't be too many warnings." Haha, WRONG.
| it('treats numeric frame names as strings', function() { | ||
| var result = Plots.computeAPICommandBindings(gd, 'animate', [[8]]); | ||
|
|
||
| expect(result).toEqual([{type: 'layout', prop: '_currentFrame', value: '8'}]); |
| }).catch(fail).then(done); | ||
| }); | ||
|
|
||
| it('updates frames referenced by number', function(done) { |
| // Since it's sometimes necessary to do deep digging into frame data, | ||
| // we'll consider it not 100% impossible for nulls or numbers to sneak through, | ||
| // so check when casting the name, just to be absolutely certain: | ||
| var stringName = newFrame.name ? newFrame.name.toString() : null; |
There was a problem hiding this comment.
Ha. 'string'.toString() is a thing. How useful!
There was a problem hiding this comment.
flushing all falsy names to null is a nice addition maintenance-wise.
There was a problem hiding this comment.
we could maybe have
var nameIn = (newFrame.name === 'number') ? newFrame.name.toPrecision(1) : newFrame.name;
var stringName = nameIn ? nameIn.toString() : null;to make sure that we only cast integer to string.
This is probably an overkill though. @rreusser I'll let decide what's best.
There was a problem hiding this comment.
I don't have strong feelings. I feel like you kinda get what you deserve if you're going to be dealing in floating point lookups, so I think maybe it's best to let the chips fall where they may and a different fixed point cuttoff is somewhat arbitrary unless we get fancy with a sprintf library or something.
And yeah, one subtlety: String(name) is definitely not the same as name.toString():
Which is why the extra gymnastics to ensure nothing is falsey.
|
💃 looks good to me. |
This PR adds some casting and fixes some checks to ensure that frames with number names are handled correctly. Technically it could break compatibility with current plots, but only in the sense that the current behavior is broken. This seems like an extremely common problem based on what I've observed.