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

Expose a new handle(externalURL) function #165

Merged
merged 4 commits into from
Nov 30, 2023

Conversation

olivaresf
Copy link
Member

@olivaresf olivaresf commented Nov 21, 2023

I found two issues that I think need solving in TurboNavigationHierarchyController.openExternal(url: URL, navigationType: NavigationStackType).

  1. Its behavior is hard-coded. It should be moved up to the sensible defaults of TurboNavigator.
  2. It cannot handle external URLs that are not http/https nor available to UIApplication (for example, a deeplink to another app).

For the hard-coded behavior, I've moved the behavior to TurboNavigator and exposed another function in TurboNavigatorDelegate, handle(externalURL:) -> ExternalURLNavigationAction. The default I chose is to send it to the system so it opens Safari (the app). The framework consumer may choose to open it in-app via a SafariController or it can choose to handle it on its own, maybe alerting the user before navigating away.

The delegation pattern solves handling non-http/https URLs. We'll report all external URLs to TurboNavigator's delegate, http/https or otherwise.

Again, I'm opening a draft so we can discuss it. Happy to change the approach and add unit tests once we're ok with it.

@svara
Copy link
Contributor

svara commented Nov 21, 2023

I agree with the issues that @olivaresf pointed out. The current behavior of openExternal(url: URL, navigationType: NavigationStackType) works well if we strictly follow the rules. However, with deep links, we have to define a custom URL scheme to open them, which we sometimes don't want to do.
I like this proposal because it offers the flexibility and control we need to handle specific cases.

safariViewController.preferredControlTintColor = .tintColor
}

rootViewController.present(safariViewController, animated: true)
Copy link
Member

@joemasilotti joemasilotti Nov 21, 2023

Choose a reason for hiding this comment

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

If we are presenting then this needs to be activeNavigationController in case we are already in a modal context.

Which makes me wonder if we should only expose that property and remove the idea of "root". The active one should always be used from an external API perspective.

@joemasilotti
Copy link
Member

I like this approach a lot! I added a comment that should be a quick fix.

I also like that we aren't taking on the responsibility of canOpenURL(_:) and leaving that to be handled, optionally, by the developer.

@olivaresf
Copy link
Member Author

I like this approach a lot! I added a comment that should be a quick fix.
I also like that we aren't taking on the responsibility of canOpenURL(_:) and leaving that to be handled, optionally, by the developer.

Thanks! There's been a lot on my plate, but I'm circling back to this PR tomorrow to fix that issue + any unit tests that are failing.

@olivaresf olivaresf marked this pull request as ready for review November 30, 2023 15:16
@olivaresf
Copy link
Member Author

@svara @joemasilotti this is good to go, I believe. Hit subscribe and smash that approve button pls 😇

Copy link
Contributor

@svara svara left a comment

Choose a reason for hiding this comment

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

This is a great, and a very useful improvement @olivaresf 👏🏻

@olivaresf olivaresf merged commit cd4f71e into turbo-navigator Nov 30, 2023
1 check passed
@olivaresf olivaresf deleted the turbo-navigator-handle-open-externalURL branch November 30, 2023 19:46
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.

3 participants