8000 Fix number named frames by rreusser · Pull Request #1236 · plotly/plotly.js · GitHub
[go: up one dir, main page]
Skip to content

Fix number named frames #1236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 7, 2016
Merged

Fix number named frames #1236

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0ff634
Basic fixes for making number-named frames work better
rreusser Dec 7, 2016
14f4874
Prevent crazy excessive warnings, just in case
rreusser Dec 7, 2016
7aca3c4
Add additional casting
rreusser Dec 7, 2016
0b3bdca
Fix null/undefined frame assumption
rreusser Dec 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add additional casting
  • Loading branch information
@rreusser
rreusser committed Dec 7, 2016
commit 7aca3c4ec4741909c28037f19261f0ea5fa09161
8 changes: 6 additions & 2 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2327,7 +2327,11 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
var newFrame = trans._currentFrame = trans._frameQueue.shift();

if(newFrame) {
gd._fullLayout._currentFrame = newFrame.name;
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha. 'string'.toString() is a thing. How useful!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flushing all falsy names to null is a nice addition maintenance-wise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fun

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author
@rreusser rreusser Dec 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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():

screen shot 2016-12-07 at 16 45 07

Which is why the extra gymnastics to ensure nothing is falsey.

gd._fullLayout._currentFrame = stringName;

trans._lastFrameAt = Date.now();
trans._timeToNext = newFrame.frameOpts.duration;
Expand All @@ -2349,7 +2353,7 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
});

gd.emit('plotly_animatingframe', {
name: newFrame.name,
name: stringName,
frame: newFrame.frame,
animation: {
frame: newFrame.frameOpts,
Expand Down