Moved WindowViewControllable and WindowSceneViewControllable to Nodes by tinder-caiofonseca · Pull Request #318 · Tinder/Nodes

tinder-caiofonseca · 2023-02-23T20:56:45Z

Current versions of WindowViewControllable and WindowSceneViewControllable are in the Nodes/genesis.yml file.

Moved these two protocols to /Nodes/Sources/Nodes/WindowViewControllable and /Nodes/Sources/Nodes/WindowSceneViewControllable, and the respective conformances of UIWindow and UIWindowScene to /Nodes/Sources/Nodes/UIKit/UIWindow+WindowViewControllable and /Nodes/Sources/Nodes/UIKit/UIWindowScene+WindowSceneViewControllable.

Sources/Nodes/ViewControllable/ViewControllable.swift

tinder-garricnahapetian · 2023-02-23T21:50:40Z

Should we put WindowViewControllable and WindowSceneViewControllable into their own files? If not, just curious why...

…esis file to Nodes

tinder-caiofonseca · 2023-02-23T23:24:47Z

Sources/Nodes/ViewControllable/WindowViewControllable.swift

+
+/**
+ * The interface used for injecting a user interface into a `Flow` instance to limit the available API,
+ * to avoid the use of UI frameworks within the `Flow` instance and to facilitate testing.


Kept the same description as ViewControllable as it seemed appropriate. Let me know if the team has a better suggestion.

tinder-caiofonseca · 2023-02-23T23:25:48Z

Sources/Nodes/ViewControllable/WindowSceneViewControllable.swift

+
+/**
+ * The interface used for injecting a user interface into a `Flow` instance to limit the available API,
+ * to avoid the use of UI frameworks within the `Flow` instance and to facilitate testing.


Kept the same description as ViewControllable but I'm not sure whether it is appropriate.

tinder-cfuller

Thank you for doing this 👍

tinder-cfuller · 2023-02-26T19:27:29Z

Sources/Nodes/ViewControllable/UIKit/UIWindow+WindowViewControllable.swift

+    /// Presents a ``ViewControllable`` instance.
+    /// - Parameter viewControllable: The ``ViewControllable`` instance to present.


We have been placing an extra line between the various "sections" of our documentation comments for readability.

Suggested change

/// Presents a ``ViewControllable`` instance.

/// - Parameter viewControllable: The ``ViewControllable`` instance to present.

/// Presents a ``ViewControllable`` instance.

///

/// - Parameter viewControllable: The ``ViewControllable`` instance to present.

Oh, missed that! I just pressed option+command+? and let Xcode generate the skeleton for me. Will update it!

tinder-cfuller · 2023-02-26T19:34:10Z

Sources/Nodes/ViewControllable/UIKit/UIWindowScene+WindowSceneViewControllable.swift

+
+extension UIWindowScene: WindowSceneViewControllable {
+
+    /// Creates a ``WindowViewControllable`` instance and associates it with the current scene object.


Suggested change

/// Creates a ``WindowViewControllable`` instance and associates it with the current scene object.

/// Creates a ``WindowViewControllable`` instance and associates it with the scene.

tinder-cfuller · 2023-02-26T19:35:09Z

Sources/Nodes/ViewControllable/UIKit/UIWindowScene+WindowSceneViewControllable.swift

+    /// Creates a ``WindowViewControllable`` instance and associates it with the current scene object.
+    /// - Returns: The ``WindowViewControllable`` instance created.


We have been placing an extra line between the various "sections" of our documentation comments for readability.

Suggested change

/// Creates a ``WindowViewControllable`` instance and associates it with the current scene object.

/// - Returns: The ``WindowViewControllable`` instance created.

/// Creates a ``WindowViewControllable`` instance and associates it with the scene.

///

/// - Returns: The ``WindowViewControllable`` instance created.

tinder-cfuller · 2023-02-26T19:42:39Z

Sources/Nodes/ViewControllable/WindowSceneViewControllable.swift

+import UIKit
+
+/**
+ * The interface used for injecting a user interface into a `Flow` instance to limit the available API,


Maybe something like this?

Suggested change

* The interface used for injecting a user interface into a `Flow` instance to limit the available API,

* The interface used for injecting a window scene into a `Flow` instance to limit the available API,

tinder-cfuller · 2023-02-26T19:43:07Z

Sources/Nodes/ViewControllable/WindowSceneViewControllable.swift

+    /// Creates a ``WindowViewControllable`` instance.
+    /// - Returns: The ``WindowViewControllable`` instance created.


Please see comments above.

tinder-cfuller · 2023-02-26T19:43:59Z

Sources/Nodes/ViewControllable/WindowSceneViewControllable.swift

+
+#else
+
+public protocol WindowSceneViewControllable: AnyObject {}


We do not need the else case here.

Suggested change

#else

public protocol WindowSceneViewControllable: AnyObject {}

tinder-cfuller · 2023-02-26T19:44:36Z

Sources/Nodes/ViewControllable/WindowViewControllable.swift

+import UIKit
+
+/**
+ * The interface used for injecting a user interface into a `Flow` instance to limit the available API,


Maybe something like this?

Suggested change

* The interface used for injecting a user interface into a `Flow` instance to limit the available API,

* The interface used for injecting a window into a `Flow` instance to limit the available API,

tinder-cfuller · 2023-02-26T19:44:53Z

Sources/Nodes/ViewControllable/WindowViewControllable.swift

+    /// Presents a ``ViewControllable`` instance.
+    /// - Parameter viewControllable: The ``ViewControllable`` instance to present.


Please see comments above.

tinder-cfuller · 2023-02-26T19:45:05Z

Sources/Nodes/ViewControllable/WindowViewControllable.swift

+
+#else
+
+public protocol WindowViewControllable: AnyObject {}


We do not need the else case here.

Suggested change

#else

public protocol WindowViewControllable: AnyObject {}

tinder-garricnahapetian · 2023-02-28T18:50:46Z

Sources/Nodes/ViewControllable/WindowViewControllable.swift

+    /// Presents a ``ViewControllable`` instance.
+    /// 
+    /// - Parameter viewControllable: The ``ViewControllable`` instance to present.
+    func present(_ viewControllable: ViewControllable)


Since we ultimately set window.rootViewController and call makeKeyAndVisible, I wonder if there's a better name for this method so that it sets the expectation of what conformers should do. The name sounds more like view controller present. But the implementation is more like contain or...?

This name is intentional. Callers of this method only care that their view controller is presented. And there is only one conformer that we need to be concerned with and it knows what to do.

Would it be silly for a Nodes developer to think - because the name is present - that setting certain presentation properties¹ on the presented view controller would cause a change in the presentation?

¹For example, transitioningDelegate or modalPresentationStyle.

I cannot say whether that would be silly or not, but I do not see that as any different than calling a custom present method that overrides those properties to different values - in either case, the internal implementation of the present method has the final say on presentation.

tinder-garricnahapetian

Great! Perhaps we could improve the docs, but I dont have any clear suggestions yet. Also, did you test that the quick start project still works as expected after this change?

tinder-cfuller

🚀

tinder-caiofonseca · 2023-02-28T19:16:29Z

Great! Perhaps we could improve the docs, but I dont have any clear suggestions yet. Also, did you test that the quick start project still works as expected after this change?

Yeah, I did test the quick project pointing to Nodes locally.

tinder-cfuller

🚀

tinder-garricnahapetian · 2023-03-13T22:29:37Z

@tinder-caiofonseca @tinder-cfuller Our QuickStart workflow uses our local copy of genesis.yml but points to a released version of Nodes. This means that our QuickStart projects are brittle. This PR actually breaks our QuickStart since those types are removed from the genesis file but not available in 0.0.14. The workaround is to checkout 0.0.14 or modified the project to point to our local copy of Nodes. I'm not sure what the long term solution is, if any to make this more robust. Mentioning so that we can discuss.

tinder-cfuller · 2023-03-13T23:39:43Z

@tinder-caiofonseca @tinder-cfuller Our QuickStart workflow uses our local copy of genesis.yml but points to a released version of Nodes. This means that our QuickStart projects are brittle. This PR actually breaks our QuickStart since those types are removed from the genesis file but not available in 0.0.14. The workaround is to checkout 0.0.14 or modified the project to point to our local copy of Nodes. I'm not sure what the long term solution is, if any to make this more robust. Mentioning so that we can discuss.

This is expected behavior. Local development requires using development versions of Nodes instead of the release version.

tinder-caiofonseca added the templates Templates label Feb 23, 2023

tinder-caiofonseca requested a review from tinder-cfuller as a code owner February 23, 2023 20:56

tinder-caiofonseca self-assigned this Feb 23, 2023

tinder-caiofonseca commented Feb 23, 2023

View reviewed changes

Sources/Nodes/ViewControllable/ViewControllable.swift Outdated Show resolved Hide resolved

Sources/Nodes/ViewControllable/ViewControllable.swift Outdated Show resolved Hide resolved

tinder-caiofonseca closed this Feb 23, 2023

tinder-caiofonseca reopened this Feb 23, 2023

Moved WindowViewControllable and WindowSceneViewControllable from gen…

36a0305

…esis file to Nodes

tinder-caiofonseca force-pushed the add-windowvc-andwindowscenevc-to-nodes branch from eb8928a to 36a0305 Compare February 23, 2023 23:22

tinder-caiofonseca commented Feb 23, 2023

View reviewed changes

tinder-cfuller reviewed Feb 26, 2023

View reviewed changes

Improved documentation as code review suggested

b1ecfcf

tinder-caiofonseca requested review from seppesnoeck, tinder-SamMarshall, tinder-cfuller, tinder-colecampbell, tinder-emanharoutunian, tinder-garricnahapetian and tinder-matthewmourlam February 28, 2023 18:37

tinder-garricnahapetian reviewed Feb 28, 2023

View reviewed changes

tinder-garricnahapetian previously approved these changes Feb 28, 2023

View reviewed changes

tinder-cfuller previously approved these changes Feb 28, 2023

View reviewed changes

Removed line trailing whitespace

4820072

tinder-caiofonseca dismissed stale reviews from tinder-cfuller and tinder-garricnahapetian via 4820072 February 28, 2023 19:20

tinder-caiofonseca requested review from tinder-cfuller and tinder-garricnahapetian February 28, 2023 19:32

tinder-garricnahapetian approved these changes Feb 28, 2023

View reviewed changes

tinder-cfuller approved these changes Mar 1, 2023

View reviewed changes

tinder-caiofonseca merged commit 0ae9729 into main Mar 1, 2023

tinder-caiofonseca deleted the add-windowvc-andwindowscenevc-to-nodes branch March 1, 2023 17:16

tinder-cfuller added framework Framework and removed templates Templates labels Aug 10, 2023

tinder-cfuller unassigned tinder-caiofonseca Nov 12, 2024

		/// Presents a ``ViewControllable`` instance.
		/// - Parameter viewControllable: The ``ViewControllable`` instance to present.


		extension UIWindowScene: WindowSceneViewControllable {

		/// Creates a ``WindowViewControllable`` instance and associates it with the current scene object.

		/// Creates a ``WindowViewControllable`` instance and associates it with the current scene object.
		/// - Returns: The ``WindowViewControllable`` instance created.

	* The interface used for injecting a user interface into a `Flow` instance to limit the available API,
	* The interface used for injecting a window scene into a `Flow` instance to limit the available API,


		#else

		public protocol WindowSceneViewControllable: AnyObject {}

Conversation

tinder-caiofonseca commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tinder-garricnahapetian commented Feb 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinder-caiofonseca Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinder-cfuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinder-garricnahapetian left a comment

Choose a reason for hiding this comment

Uh oh!

tinder-cfuller left a comment

Choose a reason for hiding this comment

Uh oh!

tinder-caiofonseca commented Feb 28, 2023

Uh oh!

tinder-cfuller left a comment

Choose a reason for hiding this comment

Uh oh!

tinder-garricnahapetian commented Mar 13, 2023

Uh oh!

tinder-cfuller commented Mar 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tinder-caiofonseca commented Feb 23, 2023 •

edited

Loading

tinder-caiofonseca Feb 23, 2023 •

edited

Loading