Skip to content
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.

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

Better axis range animations #2788

Merged
merged 5 commits into from
Jul 11, 2018
Merged

Better axis range animations #2788

merged 5 commits into from
Jul 11, 2018

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Jul 6, 2018

Axis range animations like https://codepen.io/plotly/pen/GjkZNk (which is featured on https://plot.ly/javascript/animations/) are broken on master ever since #2579 (the cartesian d3-iomatic push, again):

peek 2018-07-06 17-20

This PR makes that example run smoothly again:

peek 2018-07-06 17-22

but what's more exciting, this fix -- along with commit c708190 -- allows us to smoothly animate axis ranges on cartesian subplots with traces other than scatter. For example a bar chart:

peek 2018-07-06 17-23

etpinard added 4 commits July 6, 2018 17:15
... by skipping Cartesian.plot step when no trace data need to
    be transitioned.
... and other traces that require a _module.setPositions call
@etpinard etpinard added bug something broken status: reviewable labels Jul 6, 2018
@etpinard
Copy link
Contributor Author

etpinard commented Jul 6, 2018

cc @alexcjohnson


var calls = Registry.call.calls.all();
expect(calls.length).toBe(1, 'just one Registry.call call');
expect(calls[0].args[0]).toBe('relayout', 'called Registry.call with');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a pretty low-level assertion, not clear to me that it would catch breaks other than the exact change you made here. Would it be possible to do something like:

    var animatePromise = Plotly.animate(...)
    expect(stuff).toBe(nearWhereItStarted)
    return animatePromise;
})
.then(function() {
    expect(stuff).toBe(nearWhereItFinishes);
    expect(theFullDataAndRegistryCallStuff);

ie something about the scatter marker and bar positions being updated async that would have failed before this change?

Copy link
Contributor Author

@etpinard etpinard Jul 9, 2018

Choose a reason for hiding this comment

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

ie something about the scatter marker and bar positions being updated async that would have failed before this change?

The marker and bar position does not change, it's the subplot and clip rect translate transforms that change during axis range transitions via Cartesian.transitionAxes (which mimics cartesian dragbox logic).

Better test added in -> 19300f9

@alexcjohnson
Copy link
Collaborator

Awesome find, that we get a whole new class of animations with this little change!!! 🎉 🏆

@alexcjohnson
Copy link
Collaborator

💃

@etpinard etpinard merged commit 9a92782 into master Jul 11, 2018
@etpinard etpinard deleted the better-layout-animations branch July 11, 2018 21:06
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