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

transition delay update #1116

Closed
wants to merge 4 commits into from
Closed

Conversation

mbustosorg
Copy link
Contributor

Adding the ability to specify the transition delay in addition to transition duration. This allows you to create some more interesting transition flows by adjusting when the transitions would start.

  .transitionDelay(function (d,i) {
  return Math.floor((Math.random() * 1000) + 1);
   })

This creates a random 0-1 second delay which can give the transition another dimension.

@mbustosorg mbustosorg mentioned this pull request Mar 11, 2016
@mbustosorg
Copy link
Contributor Author

Just realized this is failing. Fixing now.

@gordonwoodhull
Copy link
Contributor

@mbustosorg wrote :

I'm having trouble with the tests. I'm getting the specs to run fine in
the browser but am seeing:

Error caught from PhantomJS. More info can be found by opening the Spec
Runner in a browser.

return selections;
dc.transition = function (selections, duration, delay, callback, name) {
if (dc.disableTransitions || duration <= 0 || duration === undefined || delay <= 0 || delay === undefined) {
return selections;
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this enforce a delay? I would think a delay of zero is valid and should still invoke the transition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll update it.

@gordonwoodhull
Copy link
Contributor

Hrrmmm, the build system can be finicky, especially on Travis... and we haven't even gotten to the Sauce Labs cross platform tests. :-S

Do you mean it's also failing on your local machine? I'm not at the computer today but I can give it a shot tomorrow.

@mbustosorg
Copy link
Contributor Author

It's behaving the same way on my machine. Using --force shows all the tests succeeding and running it in the browser shows the same (all success). I'm unable to figure out what the Error from Phantom JS is.

@gordonwoodhull
Copy link
Contributor

In that case, it's likely that one of our development dependencies has changed. This is all built on shifting sands. I'll take a look tomorrow.

@@ -108,21 +108,26 @@ describe('dc.core', function () {
duration: function () {
return this;
}
delay: function () {
return this;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it's simply a syntax error in the source here

@gordonwoodhull
Copy link
Contributor

You can run grunt lint to do a syntax check.

It will also point out that you're using tabs when the style here is to use spaces. (There's a commit hook to check this stuff but I'm not sure if it always gets installed automatically.)

Finally, before I merge this I'd like to see if it's possible to make dc.transition backward-compatible - it's not a documented function but it may be in use by folks out in the world (in derivative libraries for example). I think it may be possible to do some clever type-checking to see if delay has been omitted. I'll look at this before merging.

Otherwise, this seems like a nice general-purpose addition and a pretty clear way to open up another of d3's parameters. Thanks!

@mbustosorg
Copy link
Contributor Author

Ok! Thanks for the hint. Tests passing now and I've fixed the formatting issues.

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Nov 24, 2016
@gordonwoodhull
Copy link
Contributor

Thanks @mbustosorg! Merged for 2.0 beta 33.

gordonwoodhull added a commit that referenced this pull request Nov 26, 2016
in preparation for #1116

this was a confusing parameter, invoked when it was decided that there
would be a transition. since we used it in exactly the case where we
were calling attrTween, and we already had to check that in another
place, seems clearer just to check that every time.

(it was also going to make it impossible to add an int/function-typed
parameter)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants