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

An alternative to configuring delay values #62

Closed
tropperstyle opened this issue Aug 17, 2015 · 33 comments
Closed

An alternative to configuring delay values #62

tropperstyle opened this issue Aug 17, 2015 · 33 comments

Comments

@tropperstyle
Copy link

We're trying to automatically find the best timing for updating the view and we will get better at it in future releases, but you know best what kind of content is being loaded during the animation.
http://plugins.telerik.com/cordova/plugin/native-page-transitions

That sounds great, but that's actually pretty hard to do, and will always be error prone. As an alternative to tweaking delay values to sync animation and content changes, it would be great if the screenshot process and animation start time could be separated out and the latter manually invoked. I use a virtual dom framework and I know exactly when the new content is redrawn and ready to go, but the exact time in ms will not always be so consistent across devices.

I suggest something along these lines:

// This would initialize options and grab the screenshot but NOT perform or schedule any animation
var animation = window.plugins.nativepagetransitions.slide({ delay: "manual" });

// Manually call this whenever you are ready. Invokes animation with prior configuration
animation.run();
@EddyVerbruggen
Copy link
Contributor

Excellent idea. If anyone want to pick this up, go ahead. Alternatively you'll need to wait a while until I have time to pick up this great suggestion.

@tropperstyle
Copy link
Author

I took some time to play with the code, but as someone with zero native ios/android/windows experience, it is a pretty daunting task. Hopefully someone can pick this up soon, as I think it would add huge value to the plugin and the overall cordova ecosystem. Coupled with your flavor of virtual-dom framework, this plugin is a killer, killer combination for creating cross-platform apps in javascript that are practically indistinguishable from fully native apps.

@EddyVerbruggen
Copy link
Contributor

Hey @tropperstyle which platform would you be interested in testing this with first?

@tropperstyle
Copy link
Author

I am currently working with iOS

@Titoine
Copy link

Titoine commented Aug 31, 2015

This feature is exactly what this plugin need to be perfect. Hope to see it implemented soon. I work both on android and iOS and I would be glad to test it.

ps: thanks for this awesome plugin. Really, this plugin is amazing, hybrid app need plugin like that (do you have a donation link ?).

@deebloo
Copy link

deebloo commented Sep 27, 2015

+1

@deebloo
Copy link

deebloo commented Sep 27, 2015

Still looking for help with this?

@NgYueHong
Copy link

+1
Really hope to see this feature implemented soon.

@EddyVerbruggen
Copy link
Contributor

Hey guys, I've now added this feature for iOS's slide transition. Android will follow real soon, but I thought I'd push it so you can give it a spin.

Usage: pass iosdelay: -1 to the slide transition and the plugin won't slide, but will make the screenshot and load the new page (without the user noticing it). Passing a delay < 0 will invoke the success callback immediately (where it would normally wait until the transition is finished).

When using an iosdelay < 0 you are responsible for calling the new executePendingTransition function like this:

window.plugins.nativepagetransitions.executePendingTransition();

// or with the optional callbacks:
window.plugins.nativepagetransitions.executePendingTransition(
  function (msg) {console.log("success: " + msg)}, // called when the animation has finished
  function (msg) {alert("error: " + msg)} // called in case or touble
);

Only available in the master repo, you'll get 0.4.3-dev when you install that version.

@NgYueHong
Copy link

@EddyVerbruggen Happy to see you working on it. Will test on it when the Android version is ready.

@EddyVerbruggen
Copy link
Contributor

@NgYueHong Android support for slide has also been added, please give it a spin, see the example code above.

@kentmw
Copy link

kentmw commented Oct 13, 2015

@EddyVerbruggen, would it make sense to have an option that would either generate or take a promise that when resolved will slide the new view in? I don't see the need to have to invoke two methods when I can just resolve a promise when I'm done.

Something like:

var deferred = new Deferred();
var animation = window.plugins.nativepagetransitions.slide({ delay: "manual", animatePromise: deferred.promise() });
// do my thing.
deferred.resolve();

or even:

var animation = window.plugins.nativepagetransitions.slide({ delay: "manual", 
  beforeAnimatePromise: function() {
    var deferred = new Deferred();
    // do my async heavy lifiting then call deferred.resolve();
    return deferred.promise();
  }
});

hubspot does something similar with their beforeShowPromise in http://github.hubspot.com/shepherd/

EDIT: while this is definitely how I want this to work, I understand that we are sending these options to the native code and resolving the deferred object in the js side, doesn't carry over into the native code. I'll look into seeing if you can use promises across native/js code. My guess is that you can't.

@EddyVerbruggen
Copy link
Contributor

Hook it up like you want, I don't see what problem it solves in this context.

@kentmw
Copy link

kentmw commented Oct 13, 2015

Okay, easy add on to your code.

NativePageTransitions.prototype.slide = function (options, onSuccess, onError) {
 ...
if (options.resolveAfter) {
  var promise = _.result(options.resolveAfter); // this will pull the promise or invoke the function to get the promise
  promise.done(function() {
    this.executePendingTransition();
  });
}

@kentmw
Copy link

kentmw commented Oct 13, 2015

You can also use the presence of the promise to determine whether or not to hold off executing the transition immediately instead of playing games with the delays. Also, you don't have to publically expose the new method executePendingTransition as it can be a private helper method inside the www/NativePageTransitions.js

@EddyVerbruggen
Copy link
Contributor

And that works for everyone?

@kentmw
Copy link

kentmw commented Oct 13, 2015

I have time to work on this today. Permission to attempt a pull request?

@EddyVerbruggen
Copy link
Contributor

Of course and I really appreciate it.

Try to do only slide and see if folks like it.

@tropperstyle
Copy link
Author

I'll chime in... I think the API can be simplified even further. Instead of adding yet another option like resolveAfter - why not just extend the (ios|android|winphone)delay options to take a Promise. This has the benefit that each transition on each OS can be cleanly added as a standalone feature. It is much clearer if your current transition on your current platform supports the executePendingTransition queue.

@kentmw
Copy link

kentmw commented Oct 13, 2015

Haven't yet gotten to resolveAfter, which it sounds like is a good thing since we're still discussing possible API's. But I did create a pull request for android to clean up the transition types: #77

I'd be okay with making the delay option take a number or a promise. Do we want number, promise, or function that returns a promise?

@tropperstyle
Copy link
Author

Thinking about it some more. One problem that came up immediately is that you will want to make sure the transition method is invoked before your async code. I think we can ditch thinking about promises, and use a thunk-like function.

// Updating the href triggers a route change in my application code, so this fires DOM reconstruction.
window.plugins.nativepagetransitions.slide({
    href: '#/some/route',
    iosdelay: function(animate) {
        // invoke animate function whenever you are ready
        afterNextRedraw(function() { animate() });
    }
});

This gives you all the functionality without worrying about device support for the Promise spec, and without any extra API methods or additional options.

@kentmw
Copy link

kentmw commented Oct 13, 2015

That sounds fine. @tropperstyle is that similar to express's next function?

kentmw added a commit to kentmw/NativePageTransitions that referenced this issue Oct 13, 2015
… of integer to control when animating pending transition happens
@kentmw
Copy link

kentmw commented Oct 13, 2015

My pull request now includes this update. Up for hearing how to make it cleaner if necessary.
Tested successfully with:

window.plugins.nativepagetransitions.slide({
  androiddelay: function(animate) {
    setTimeout(function() { 
      animate();
    }, 1000)
  }
});

@NgYueHong
Copy link

@EddyVerbruggen I have tested on my Android devices by setting androiddelay = -1 and execute window.plugins.nativepagetransitions.slide({"direction": "left"}); when user click on a link. When the next page is loaded and ready, then run window.plugins.nativepagetransitions.executePendingTransition();.

It works great. Exactly how I wanted. 100% control when to capture screenshot and animate. No more animate too soon or too late issue!!!! Great work @EddyVerbruggen . Really appreciate it. Will share with others about this.

Hope to see it works on other transition as well soon. Thanks.

@EddyVerbruggen
Copy link
Contributor

Thanks for testing this new feature all!

I have now added the iosdelay and androiddelay -1 feature to fade, drawer, flip and curl (iOS) as well.

Available in 0.5.0.

Enjoy,
Eddy

@kentmw
Copy link

kentmw commented Oct 20, 2015

Hey @EddyVerbruggen what's the state of my pull request for this issue? Did you look at it?

kentmw pushed a commit to kentmw/NativePageTransitions that referenced this issue Oct 20, 2015
kentmw pushed a commit to kentmw/NativePageTransitions that referenced this issue Oct 20, 2015
kentmw pushed a commit to kentmw/NativePageTransitions that referenced this issue Oct 20, 2015
kentmw pushed a commit to kentmw/NativePageTransitions that referenced this issue Oct 20, 2015
kentmw pushed a commit to kentmw/NativePageTransitions that referenced this issue Oct 20, 2015
kentmw added a commit to kentmw/NativePageTransitions that referenced this issue Oct 20, 2015
… of integer to control when animating pending transition happens
@kentmw
Copy link

kentmw commented Oct 20, 2015

@EddyVerbruggen, I tested on iOS and was able to see changes in the html before executing the pending transition. It's like the screenshot was not overlayed during the first step. This is how I tested:

window.plugins.nativepagetransitions.slide({
  iosdelay: -1, 'direction': 'left'
});
$('.foo').css('height', '100px');
setTimeout(function() {window.plugins.nativepagetransitions.executePendingTransition()}, 1000);

And the height was changed, then the slide transition happend. Are we sure that the screenshot is actually used? I think this issue needs to be reopened or a new bug opened.

Btw, this error happens in my pull request and master.

NOTE: Android works perfectly.

@EddyVerbruggen
Copy link
Contributor

@kentmw That's expected behavior. Cordova plugins are asynchronous, so the call to slide will not block the JS thread. Long before the screenshot is taken the css change has kicked in.

That's also the reasons I've added the href param to the plugin API in since version 0.0.1. So you can be sure the screenshot has been taken before the webview navigates to the new div/page.

If you want to test this with a simple css change (without passing the href param to the plugin) you should wrap the css change in a setTimeout with a small delay. It is very hard to time that delay of course.

@kentmw
Copy link

kentmw commented Oct 20, 2015

@EddyVerbruggen, I actually had that same thought. And I did test that:

window.plugins.nativepagetransitions.slide({
  iosdelay: -1, 'direction': 'left'
});
setTimeout(function() {$('.foo').css('height', '100px');}, 1000);
setTimeout(function() {window.plugins.nativepagetransitions.executePendingTransition()}, 2000);

Yields the same bad results. 1 second should be more than enough time to overlay the screenshot.

EDIT: same with 10/20 seconds
EDIT 2:
With a delay actually set things work:

window.plugins.nativepagetransitions.slide({
  iosdelay: 1000
});
$('.foo').css('height', '100px');

This doesn't show any changes even without a timeout on the css change. Do you think maybe the call to [UIView animateWithDuration:duration] is actually the call that is hiding any rendering, not the overlaying screenshot?

@EddyVerbruggen
Copy link
Contributor

Aha, that's an interesting observation! Let me dive into that..

@EddyVerbruggen
Copy link
Contributor

@kentmw You found a bug sir, have a beer 🍺

Slide right was ok, slide left wasn't. It is in 0.5.1.

@kentmw
Copy link

kentmw commented Oct 20, 2015

Yay! Thanks for identifying where the problem was and adding the fix quickly.

EDIT: beautiful. works great after testing. Now the executePendingTransition also works for left slides.

@kentmw
Copy link

kentmw commented Oct 21, 2015

Here's a little interesting piece of info to keep in mind when thinking about animations and custom delays:
http://stackoverflow.com/questions/33245106/uiview-animate-with-delay-blocks-ui-interactions
During the pre-animate delay it will, in fact, block UI interactions/re-rendering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants