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

Allow a nav controller to be pushed into a modal stack #205

Closed
wants to merge 3 commits into from

Conversation

olivaresf
Copy link
Member

I found a scenario we hadn't considered with TurboNavigator: there are times where pushing/replacing the modal controller with a navigation controller is expected. For example, presenting an [EKEventEditViewController](https://developer.apple.com/documentation/eventkitui/ekeventeditviewcontroller) so the user can add a new event to their calendar is currently not possible because EKEventEditViewController inherits from UINavigationController.

With the current implementation, TurboNavigator crashes when trying to push a EKEventEditViewController on a modal stack because it's disallowed to set a navigation controller as part of another navigation controller's view controllers (modalNavigationController.setViewControllers([controller], animated: true)).

I'm not particularly fond of the fix but I also don't see anything obviously wrong with it.

There are times where this is expected (e.g. EKEventEditViewController inherits from a nav controller and it must be presented).
@olivaresf olivaresf marked this pull request as draft April 27, 2024 04:03
@olivaresf
Copy link
Member Author

The code is ready, but I've turned this into a draft because I'd like to get opinions before moving forward.

@joemasilotti
Copy link
Member

This approach sounds solid to me. Checking for inheritance from UINavigationController doesn't seem much different than the check for UIAlertController we are already doing elsewhere.

My only request is to pull out the duplicated test setup code for making a window and moving the no longer private helper somewhere else.

Comment on lines +89 to +92
if let proposedModalNavController = controller as? UINavigationController {
proposedModalNavController.setModalPresentationStyle(via: proposal)
navigationController.present(proposedModalNavController, animated: true)
} else if navigationController.presentedViewController != nil, !modalNavigationController.isBeingDismissed {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this doesn't work when navigating with a UINavigationController instance (e.g. UIImagePickerController, or EKEventEditViewController) while in a modal presentation.

@svara
Copy link
Contributor

svara commented May 13, 2024

Thanks for bringing this up @olivaresf! Navigating with an instances of UINavigationController is a specific case, and we should definitely handle it.
The changes in this PR don't seem to be working for me.

A tangential issue I also ran into is that the current implementation doesn't allow for more than one modal. If we are on a modal screen, and the new screen requires a modal presentation, it gets pushes instead of being presented modally on the modal navigation controller.

I'll provide my feedback in an upcoming PR.

@svara
Copy link
Contributor

svara commented May 14, 2024

I'm looping @jayohms into this discussion.

@olivaresf
Copy link
Member Author

I've discussed this again with @svara and @zoejessica and for now I've decided to close this PR. We don't really have a consensus on whether this solution is moving in the right direction. Even though this is the same as the alert controller check, I'd rather we move more towards these checks being part of the path config somehow. If we have a need for this again, we can re-explore with more time.

Thanks!

@olivaresf olivaresf closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants