Feature/swiftui integration #58
Conversation
| "SAMPLE_LIST_VIEW_CONTROLLER.UIKIT_INTEGRATION.TITLE" = "UIKit Integration"; | ||
| "SAMPLE_LIST_VIEW_CONTROLLER.SWIFTUI_INTEGRATION.TITLE" = "SwiftUI Integration"; |
There was a problem hiding this comment.
Just an idea, because at first glance, I oversaw that UIKit Integration also makes use of the new SwiftUISupport Module:
What about renaming these to UIKit → SwiftUI Integration and SwiftUI → UIKit Integration?
| ) | ||
| .frame(height: 44) | ||
|
|
||
| ForEach(images, id: \.self) { url in |
There was a problem hiding this comment.
When running this, I got a warning
ForEach<Array<URL>, URL, ModifiedContent<StatefulViewRepresentable<ImageView>, _FrameLayout>>: the ID https://picsum.photos/200 occurs multiple times within the collection, this will give undefined results!
While this is just a warning now, maybe it will lead to actual problems with future SwiftUI updates. So I'd suggest to change the line to
ForEach(Array(images.enumerated()), id: \.offset) { _, url in| @@ -977,7 +1008,7 @@ | |||
| CODE_SIGN_STYLE = Automatic; | |||
| DEVELOPMENT_TEAM = DB7EUU9D79; | |||
| INFOPLIST_FILE = App/SupportingFiles/Info.plist; | |||
| IPHONEOS_DEPLOYMENT_TARGET = 10.0; | |||
| IPHONEOS_DEPLOYMENT_TARGET = 13.0; | |||
There was a problem hiding this comment.
There are 3 places in the sample project where there are still if #available checks that are presumably no longer needed with this deployment target bump. Could you remove them?
| isa = XCRemoteSwiftPackageReference; | ||
| repositoryURL = "https://github.com/JamitLabs/JamitFoundation"; |
There was a problem hiding this comment.
With the change to SSH, I got a build error for some reason that I needed to first trust the SSH server to fetch the dependency.
Is there any advantage for using SSH? If not, I'd suggest to stay with the HTTPS variant.
| requirement = { | ||
| branch = develop; | ||
| branch = "feature/swiftui-integration"; |
There was a problem hiding this comment.
I'm sure changing this back just before merging this PR is already on your mind, but just wanted to drop a reminder to make sure.
| @available(iOS 13.0, *) | ||
| /// Takes a stateful view as a generic type and wraps it into a `UIViewRepresentable`. | ||
| /// The stateful views model can be changed by changing the `model` property. | ||
| public struct StatefulViewRepresentable<ContentView: StatefulViewProtocol>: UIViewRepresentable { |
There was a problem hiding this comment.
StatefulViewRepresentable sounds a bit like a protocol, not like a struct. How about StatefulViewRepresenter?
I don't have really strong opinions on this, but maybe this is something worth discussing again (maybe with @mrylmz) before merging this PR and thereby somehow finalizing the name.
|
|
||
| @available(iOS 13.0, *) | ||
| /// Takes a swiftui view as a generic type and wraps it into a `StatefulView`. | ||
| public class SwiftUIContainerView<ContentView: View>: StatefulView<EmptyViewModel> { |
There was a problem hiding this comment.
I'm probably missing something, but at the moment I don't see how this type offers any benefits over directly using UIHostingController. Is it really needed?
| @@ -83,6 +84,11 @@ let package = Package( | |||
| name: "MessageView", | |||
| dependencies: ["JamitFoundation"], | |||
| path: "Modules/MessageView" | |||
| ), | |||
| .target( | |||
There was a problem hiding this comment.
You have probably developed by opening the Package.swift file. With this workflow, however, the new folder and files are not referenced in the Xcode project file and the pod spec isn't updated.
It would probably best to just drop CocoaPods and Carthage support (and also the Xcode project file), now that SPM performs so seamlessly. But if we keep support for them, it would be needed to also update the pod spec and the Xcode project file.
| self.hostingController = .init(rootView: contentView) | ||
|
|
||
| super.init(frame: .zero) | ||
| isUserInteractionEnabled = false |
There was a problem hiding this comment.
Maybe I'm again not getting something, but just wanted to make sure: Is this line intended to stay or is it there by mistake?
jamitJona
force-pushed
the
feature/swiftui-integration
branch
3 times, most recently
from
18487f0 to
01b8317
Compare
jamitJona
force-pushed
the
feature/swiftui-integration
branch
from
01b8317 to
961d90b
Compare
mrylmz
force-pushed
the
feature/swiftui-integration
branch
from
610261c to
52d1bc0
Compare
Adds a wrapper to integrate StatefulViews into SwiftUIViews and a wrapper to integrate SwiftUIViews as StatefulViews.