Skip to content
This repository has been archived by the owner on Jan 14, 2018. It is now read-only.

Allow left and right views to have a custom frame, giving more power to ECSlidingViewControllerLayout #244

Closed
wants to merge 5 commits into from

Conversation

hartbit
Copy link

@hartbit hartbit commented Dec 2, 2013

Small recap of modifications:

  1. I modified underLeftViewCalculatedFrame and underRightViewCalculatedFrame methods to underLeftViewCalculatedFrameForPosition: and underRightViewCalculatedFrameForPosition: to allow left and right views to control their frames when implementing ECSlidingViewControllerLayout.
  2. I fixed a bug in viewControllerForKey that war returning the wrong UITransitionContextFromViewControllerKey when the operation was ECSlidingViewControllerOperationResetFromLeft or ECSlidingViewControllerOperationResetFromRight.
  3. I updated initialFrameForViewController: and finalFrameForViewController: to call the modified calculation methods.
  4. I modified the default ECSlidingAnimationController, simplifying it thanks to the bug fixed in 2, and so not to make any assumptions concerning the frame of left and right views.
  5. This allowed me to implement a MEParallaxAnimationController in the TransitionFun example that implements a parallax effect when showing left and right views, simply by implementing ECSlidingViewControllerLayout.

…t and underRight views can have different frames for each operation

Signed-off-by: David Hart <david@atipik.ch>
…ample.

Signed-off-by: David Hart <david@atipik.ch>
Signed-off-by: David Hart <david@atipik.ch>
…t and right views that can change frame.

Signed-off-by: David Hart <david@atipik.ch>
@enriquez
Copy link
Member

enriquez commented Dec 3, 2013

Awesome, thanks for the PR. Some feedback:

  • I'd like to keep what you did for 1 and 3. Giving ECSlidingViewControllerLayout the ability to change the under view frames is great. I think a bug is introduced when getting the finalFrameForViewController: for the under view. It should return CGRectZero when the top view is resetting. You'll see this bug by running rake test.
  • PR Get the right view controller for key. #243 beat you to fixing the bug for 2. It should be a clean merge with master.
  • The default sliding animation's ending frames for the under views should be the same as the initial frame. So, I think 4 is not needed.
  • I ran TransitionFun with MEParallaxAnimationController, but it looks like the default transition. I'm probably not seeing something?

I'd like to focus this PR on the changes you have for improving ECSlidingViewControllerLayout with underLeftViewCalculatedFrameForPosition: and underRightViewCalculatedFrameForPosition:. Could you remove everything else from this PR?

I want to see where you're going with the parallax transition, but I'd like to see it as a fork or separate project. Adding it to TransitionFun is going to be difficult to maintain (it already is with the 4 transitions already there). I plan on making each of the transitions from TransitionFun into their own projects. Eventually, there'll be a list of user contributed transitions on the Wiki.

@hartbit
Copy link
Author

hartbit commented Dec 3, 2013

I'd like to keep what you did for 1 and 3. Giving ECSlidingViewControllerLayout the ability to change the under view frames is great. I think a bug is introduced when getting the finalFrameForViewController: for the under view. It should return CGRectZero when the top view is resetting. You'll see this bug by running rake test.

Can you explain why finalFrameForViewController: should return CGRectZero when the top view is resetting? In my code, I use the finalFrameForViewController: of the under views to achieve the parallax effect: my under-views move, even when resetting. If I change the finalFrame to CGRectZero, my transition will animate the under views to CGRectZero.

The default sliding animation's ending frames for the under views should be the same as the initial frame. So, I think 4 is not needed.

What you say may be our mis-understanding. If I want the under views to move while revealing and resetting, shouldn't the initial frame and final frame be different? That's how I implemented the parallax effect at least. If change ECSlidingAnimationController back to it's original implementation, I don't get any animations for free.

I ran TransitionFun with MEParallaxAnimationController, but it looks like the default transition. I'm probably not seeing something?

I had forgotten to implement the delegate method to point to the layout :) Fixed now. Check it out, perhaps you can better understand the effect I wanted to achieve.

I want to see where you're going with the parallax transition, but I'd like to see it as a fork or separate project. Adding it to TransitionFun is going to be difficult to maintain (it already is with the 4 transitions already there). I plan on making each of the transitions from TransitionFun into their own projects. Eventually, there'll be a list of user contributed transitions on the Wiki.

If you don't want to integrate it into the project, that's fine. I'll just keep it in my fork. But personally, I think it's a pitty not to grow the list of transitions in the TransitionFun project. First of all, the current transitions helped me quickly test that the modifications I made to the project did not break a transition. The more there is there, the more "testing" can be done. Secondly, if you keep the transitions with the project, it is a bit more work to maintain them. But I think that's better than having links in the wiki to transitions that may be outdated and not build with the latest version of ECSlidingViewController.

@enriquez
Copy link
Member

enriquez commented Dec 3, 2013

Can you explain why finalFrameForViewController: should return CGRectZero when the top view is resetting? In my code, I use the finalFrameForViewController: of the under views to achieve the parallax effect: my under-views move, even when resetting. If I change the finalFrame to CGRectZero, my transition will animate the under views to CGRectZero.

This is the behavior for iOS 7 custom transitions. CGRectZero signifies the view is being removed or is not on the screen. The parallax transition could've also been accomplished with a custom animation that sets the appropriate frames (and ignoring the frames returned from finalFrameForViewController:). There's nothing wrong with the custom layout you did, that works too.

I now realize there's a bug with initialFrameForViewController:. For anchoring transitions, it should return CGRectZero for both under views.

What you say may be our mis-understanding. If I want the under views to move while revealing and resetting, shouldn't the initial frame and final frame be different? That's how I implemented the parallax effect at least. If change ECSlidingAnimationController back to it's original implementation, I don't get any animations for free.

The default animation specifically does not move the under view. Considering the bug I mentioned above, it should just set the final frame for the under view from the beginning.

I see what you did now with the parallax transition. That's fantastic! The intent for TransitionFun was to demonstrate how to implement custom transitions. Separating them as their own projects will make them easily installable and usable for other users when packaged as a CocoaPod. Having different repos will make maintenance easier, since this repo won't be polluted with transition issues. As for versioning, your transition can be tied to a specific version of ECSlidingViewController. CocoaPods makes this easy with dependencies.

Great work by the way, I really like the parallax transition.

@enriquez
Copy link
Member

enriquez commented Dec 4, 2013

Fixed the issue with initialFrameForViewController: returning a frame for views not on screen d3c45c3

@hartbit
Copy link
Author

hartbit commented Dec 4, 2013

This is the behavior for iOS 7 custom transitions. CGRectZero signifies the view is being removed or is not on the screen. The parallax transition could've also been accomplished with a custom animation that sets the appropriate frames (and ignoring the frames returned from finalFrameForViewController:). There's nothing wrong with the custom layout you did, that works too.

Thanks for explanation, I was not aware of that! But there's something I still don't understand. If the finalFrameForViewController: method of UIViewControllerContextTransitioning needs to return CGRectZero for views which are not visible at the end of the transition, then my custom layout can't be correct. I return a non zero final position for the under views on resetting even if they end up being hidden. Or I am getting something wrong again?

@enriquez
Copy link
Member

enriquez commented Dec 4, 2013

The custom layout you used for the parallax transition happens to work with the default animation. The default animation simply animates from the starting frame to the ending frame, and since you customized the layout of the frames you were able to achieve the effect you wanted.

After thinking about this more, I think the parallax transition would be better implemented with a custom animation instead of a custom layout. This removes its dependency from the default animation.

Your custom animation is free to set the frames to whatever it wants to if the UIViewControllerContextTransitioning methods return CGRectZero. It should set the view's frames to any non-zero rect as defined by the UIViewControllerContextTransitioning methods. Not doing so may break bound changes and rotation, and may cause general layout issues. The iOS 7 custom transitions have problems with this. This is why I came up with custom layout.

Custom layouts should return CGRectZero when the frame is unknown or doesn't matter. It gives a hint to the animation to add/remove the view, but it depends on how the animation is implemented.

There's a lot of flexibility with customizing the transitions, so you're free to do whatever as long as it works for you. If your app is a single orientation and screen size, then you'll be able to get away with breaking more of these rules. What you did with the parallax transition is good, and it should work on iPad as well. The only caveat I can think of is that you are now responsible for handling any view controllers that have their edgesForExtendedLayout set in a custom layout (if you want to!).

You bring up some good points, I'll add some more notes to the docs about this.

@hartbit
Copy link
Author

hartbit commented Dec 5, 2013

Thanks for the length explanation! I have a much better understanding of everything now. I think I'll reimplement the parallax effect using an animation instead of a layout as soon as I get some time. Meanwhile, can you recap what you'd like me to keep in the PR?

@enriquez
Copy link
Member

enriquez commented Dec 5, 2013

Cool, I look forward to it.

Just keep the changes related to underLeftViewCalculatedFrameForPosition: and underRightViewCalculatedFrameForPosition: for this pull request. Thanks again.

@enriquez enriquez closed this in 144f0aa Jan 7, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants