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.

Sign up for GitHub

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

Add bundle parameter to route #207

Merged
merged 3 commits into from
May 9, 2024
Merged

Add bundle parameter to route #207

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
820cfa9
Add bundle parameter to route
olivaresf Apr 27, 2024
a242deb
Rename to parameters
olivaresf May 9, 2024
a95d7c1
Rename code comment
olivaresf May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Source/Turbo Navigator/TurboNavigator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,11 @@ public class TurboNavigator {
/// Convenience function to routing a proposal directly.
///
/// - Parameter url: the URL to visit
public func route(_ url: URL) {
/// - Parameter bundle: provide context relevant to `url`
public func route(_ url: URL, bundle: [String: Any]? = nil) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should go with Any? instead of [String: Any]?. This would allow to directly pass an object, instead of providing a key value pair.

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 think keeping this makes more sense as it maintains parity with Android.

let options = VisitOptions(action: .advance, response: nil)
let properties = session.pathConfiguration?.properties(for: url) ?? PathProperties()
route(VisitProposal(url: url, options: options, properties: properties))
route(VisitProposal(url: url, options: options, properties: properties, bundle: bundle))
}

/// Transforms `VisitProposal` -> `UIViewController`
Expand Down
7 changes: 6 additions & 1 deletion Source/Visit/VisitProposal.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,15 @@ public struct VisitProposal {
public let url: URL
public let options: VisitOptions
public let properties: PathProperties
public let bundle: [String: Any]?

public init(url: URL, options: VisitOptions, properties: PathProperties = [:]) {
public init(url: URL,
options: VisitOptions,
properties: PathProperties = [:],
bundle: [String: Any]? = nil) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, this will be great to have. If bundle feels like odd language for the platform, feel free to rename. Android uses bundle since it's a platform convention. On the destination side, when you extract the contents of a bundle, you use an arguments property to extract the key/value pairs. Would arguments be a better name here than you can use? It might make the intent more clear on the navigating side and the destination side.

Copy link
Member

Choose a reason for hiding this comment

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

What actually is the bundle here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If bundle feels like odd language for the platform, feel free to rename.

It doesn't feel particularly odd. We could certainly rename it to arguments or context or something similar. While I like how it's consistent with Android naming, I don't feel too strongly about it.

What actually is the bundle here?

So, for example, in HEY we have an import contacts flow that's something like this:

  1. Present contacts from the user's phone (native: CNContactPickerViewController)
  2. When user selects contacts and taps done, move to an upload progress screen (native: e.g. UploadContactsViewcontroller)

There's no easy path to move between screen 1 and 2 using TurboNavigator since screen 2 requires the contacts selected from screen 1. So we'd need to tightly couple 1 and 2 (defeats the purpose of having the path config) or pass the contacts some other way (not super clean).

Jay and I discussed this and Android has an easy way of doing this which is passing along the data required for setting up the screen alongside the route.

In the scenario above, we'd call something like route(to: "/native/contacts/upload", bundle: ["contacts" : selectedContacts]). The bundle becomes a part of the VisitProposal so when the client needs to resolve the proposal in handle(proposal: VisitProposal) they have all the data needed to setup the new client.

Copy link
Member

Choose a reason for hiding this comment

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

Ohhh! Very interesting. Thanks for the explanation.

My vote is context. The term "bundle" is a little loaded on iOS in my opinion, since it could mean the actual application bundle itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! Sure, totally missed that. I'll rename to context. 👍🏻

Copy link
Collaborator

@jayohms jayohms Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should use context, since that conflicts with the existing first-class context: "default" and context: "modal" setup in the path config. Let's find something else that makes it clear how its intended to be used. I'm also fine renaming the Android one in the future if we find a good name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point too! I'll try to find a good name and report back.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this idea!
Regarding naming, what about parameters or arguments?

Copy link
Member Author

Choose a reason for hiding this comment

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

parameters ended up being the winner. Thanks everyone!

self.url = url
self.options = options
self.properties = properties
self.bundle = bundle
}
}