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 framework users to navigate to an external URL using TurboNavigator #167

Merged
merged 4 commits into from
Jan 11, 2024

Conversation

olivaresf
Copy link
Member

@olivaresf olivaresf commented Dec 15, 2023

Exposing this function is a convenience so that, for common use cases, a framework user can open an external URL by using TurboNavigator, instead of implementing these functions themselves.

Session behavior when an external URL is found is unchanged.

Copy link
Member

@joemasilotti joemasilotti left a comment

Choose a reason for hiding this comment

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

Good idea! I made one suggestion on the documentation and the API. Let me know what you think.

Source/Turbo Navigator/TurboNavigator.swift Outdated Show resolved Hide resolved
/// - Parameters:
/// - externalURL: the URL to navigate to
/// - via: navigation action
public func open(externalURL: URL, _ via: ExternalURLNavigationAction) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public func open(externalURL: URL, _ via: ExternalURLNavigationAction) {
public func open(externalURL url: URL, via action: ExternalURLNavigationAction) {

If via isn't optional then I think we should expose the name in the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong preference. The reason I removed the label is because of call readability:

open(anExternalURL, via: .openViaSafariController)

vs.

open(anExternalURL, .openViaSafariController)

I don't mind either way. If you still feel the label works, can you commit your own suggestion? (I don't know if Github allows that, if not, I can merge it!)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for writing this out! I'm now thinking we should change the enum so usage can read more fluently.

open(externalURL, via: .safariViewController)
open(externalURL, via: .system)

If that sounds good let me know and I'll make a commit locally and push.

Copy link
Member Author

@olivaresf olivaresf Jan 3, 2024

Choose a reason for hiding this comment

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

Since we're now using ExternalURLNavigationAction in two places (handle(externalURL:) and open(externalURL:_:)) one of them will have their readability suffer.

For example, using .viaSafariViewController will read well in the former, but not the latter:

func handle(externalURL: URL) -> ExternalURLNavigationAction {
    .openViaSafariViewController
}
open(externalURL, via: .openViaSafariViewController)

vs.

func handle(externalURL: URL) -> ExternalURLNavigationAction {
    .safariViewController
}
open(externalURL, via: .safariViewController)

My preference is including openVia in the enum case names. This is such a small change that we should go with whichever. I wanted to point out though, that both use cases have different grammar.

@olivaresf
Copy link
Member Author

@joemasilotti will merge this in as-is. If we decide on a name change, we can do another PR.

@olivaresf olivaresf merged commit e908c98 into turbo-navigator Jan 11, 2024
1 check passed
@olivaresf olivaresf deleted the expose-external-url-navigation branch January 11, 2024 14:59
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.

2 participants