Skip to content

Minor fixes to allow null frames internally#1121

Merged
rreusser merged 3 commits into
masterfrom
null-frames
Nov 9, 2016
Merged

Minor fixes to allow null frames internally#1121
rreusser merged 3 commits into
masterfrom
null-frames

Conversation

@rreusser

@rreusser rreusser commented Nov 8, 2016

Copy link
Copy Markdown
Contributor

This PR adds a small test and a couple fixes to prevent plotly from breaking on null frames. This is necessary for transforms that directly add or remove frames. If they remove frames, they should avoid compressing the array so that indices don't change. This means null or undefined entries in the frame array should simply be ignored.

@etpinard etpinard added bug something broken status: in progress labels Nov 8, 2016
@etpinard etpinard added this to the v1.20.0 milestone Nov 8, 2016
@etpinard

etpinard commented Nov 8, 2016

Copy link
Copy Markdown
Contributor

fyi @rreusser you'll need to merge master to make

image

pass again.

Comment thread src/plot_api/plot_api.js
for(i = 0; i < trans._frames.length; i++) {
frame = trans._frames[i];

if(!frame) continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so, technically, not just null is special. Any non-numeric i would make trans._frames[i] falsy?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i can't be non-numeric since it's the array index. This is a check to see if the accessed frame is falsey. Perhaps a better check is if (!Lib.isPlainObject(frame)).

@etpinard

etpinard commented Nov 9, 2016

Copy link
Copy Markdown
Contributor

Making Plotly.animate and friends skip over blank frames makes sense. Thanks for cleaning that up 🍻

I wonder though, if we could expose a way through Plotly.animate to make a graph go back to its original "data" / "layout"` settings?

@rreusser

rreusser commented Nov 9, 2016

Copy link
Copy Markdown
Contributor Author

Short answer… no. I don't see any way to accomplish this without structural changes to animation. As discussed previously, I think the solution would be frames that you would toggle on/off. On each data -> fullData step, it would merge any 'on' frames. Turning the frames off would restore it to the original state.

@etpinard

etpinard commented Nov 9, 2016

Copy link
Copy Markdown
Contributor

Thanks for the reply @rreusser

Turning the frames off would restore it to the original state.

This would certainly make layout.breakpoints frames easier to deal with.

Perhaps (or not) this could as easy as adding a type key to the frame attributes. type would be an enumerated with possible values: 'sequencial' and 'toggle'.

@rreusser

rreusser commented Nov 9, 2016

Copy link
Copy Markdown
Contributor Author

Yeah, I think with all the other machinery in place and with a much better understanding of what we're after that this might actually not be so difficult. The reason this wasn't done to begin with is that one solution cannot handle both ephemeral update-and-forget-about-it updates and stateless toggle updates. I think it would be worthwhile though.

Conflicts:
	test/jasmine/tests/animate_test.js
@etpinard

etpinard commented Nov 9, 2016

Copy link
Copy Markdown
Contributor

💃

@rreusser
rreusser merged commit 2ac3dd7 into master Nov 9, 2016
@etpinard
etpinard deleted the null-frames branch November 9, 2016 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug something broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants