search

iZettle / Presentation

Presentation is an iOS Swift library for working with UI presentations in a more structured way
MIT License
72 stars 11 forks source link

Fix PushPoper animations #29

Closed sampettersson closed 5 years ago

sampettersson commented 5 years ago

I was debugging why the title of a pushed view controller just suddenly pops into it's final position without performing a transition as it should, found out that setViewControllers(vcs, animated: animated) was the culprit, so I guess something is flawed inside UIKit.

Not so sure about this fix, but it does fix the animations.

Before:

After:

mansbernhardt commented 5 years ago

Great find and work around. Have you investigated if this is an issue just for certain iOS versions or for all support ones?

sampettersson commented 5 years ago

@mansbernhardt I checked in the simulator, worked as it should in iOS 10, but since iOS 11 it doesn't.

mansbernhardt commented 5 years ago

@mansbernhardt What is the status on this PR?

sampettersson commented 5 years ago

@mansbernhardt fine for merging by me (I don't have merge rights), though the tests are failing for some reason, doesn't seem related tho 🤔

mansbernhardt commented 5 years ago

@Iamsamwhoami Do the unit tests pass locally? I can see that they fail on the build server:

ExampleUITests.ExampleUITests
  testNavigationPresentationStyle, Lost connection to the application (pid 3093).
  ExampleUITests.swift:198

Not sure why, perhaps @nataliq can shed some light?

nataliq commented 5 years ago

Hey @Iamsamwhoami @mansbernhardt Sorry for the late response, I missed this mention here! I restarted the build few times on the CI to check if it's flaky but it fails every time. After running the UI tests on the StylesAndOptions example app locally I see that the test is failing after these changes (there is a crash actually), so perhaps it is a good catch. If you run that app in the simulator the crash is reproducible every time and looks related to the change: 'Pushing the same view controller instance more than once is not supported.

sampettersson commented 5 years ago

@nataliq ah, ok! I’ll check it out.

sampettersson commented 5 years ago

I will come back to this at some point when I can figure out why the test fails 🙈