WIP Add SwiftUI Quickstart for Storage #1134

codeblooded · 2021-03-26T07:45:10Z

This is an incomplete migration of the Storage Quickstart to Swift UI. It is an active work in-progress.

To Do

Add helpful comments
Consider moving some state into the environment
Add error dialogs to UI
Spice it up with some styling and animation
Consider support for multiple images
Move to Swift target, replacing UIKit example

paulb777

Great progress!

Just a few structure comments for now. I'll review the code later.

paulb777 · 2021-03-29T21:57:31Z

storage/Podfile

@@ -1,7 +1,7 @@
 # StorageExample

 use_frameworks!
-platform :ios, '10.0'
+platform :ios, '14.4'


Is 13.0 or 14.0 possible?

Also, please factor the platform spec into the targets so we can keep 10.0 for the old quickstart.

paulb777 · 2021-03-29T21:58:29Z

storage/StorageExample.xcodeproj/project.pbxproj

@@ -11,6 +11,8 @@
 1073487D20333C27004A66D1 /* StorageExampleUITests.m in Sources */ = {isa = PBXBuildFile; fileRef = 1073487C20333C27004A66D1 /* StorageExampleUITests.m */; };


Run pod deintegrate before merging to keep the Xcode project more easily mergeable and able to support Swift PM.

paulb777 · 2021-03-29T21:59:48Z

storage/StorageExampleSwiftUI/ImagePicker.swift

@@ -0,0 +1,77 @@
+//


I know this repo isn't consistent with this, but please use the standard Firebase open source copyright like https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseStorageSwift/Sources/SwiftAPIExtension.swift#L1

ncooke3

Awesome job– this is great! 🥳

Some additional suggestions/tips:

add the Image.xcassets to the new app’s target so the new app picks up the standard qs icon
use SFSymbols when possible
check out Apple's Human Interface Guidelines (if you'd like more design inspo, Im happy to share some apps I refer to for their design)
provide a screenshot of the latest UI to the PR (I should have done this when I helped make some qs 😅 )

I helped implement qs for Auth, Remote Config, and Analytics last summer. They are in UIKit but could be helpful in with design, etc. I tried to make use of the Firebase APIs clear and copy-and-paste-able by simplifying and abstracting the other stuff away.

Some examples of areas where I tried to make Firebase API usage really clear:
• AuthViewController
• ConfigViewController

All that to say, I think the usage of Firebase APIs is really clear here. I just think this is the ultimate goal for all the qs!

Sorry, there’s a lot of comments but my hope is that they are helpful. Again, great job so far!! Feel free to ping me if you have questions/want me to try and demo one/etc

p.s. i cloned your fork to have a look around. feel free branch off of the main repo if that is any easier for you 🙂

ncooke3 · 2021-04-15T17:05:45Z

storage/StorageExampleSwiftUI/StorageExampleSwiftUIApp.swift

+ Auth.auth().signInAnonymously(completion: { (authResult, error) in
+ if let error = error {
+ print("Failed to sign in: \(error.localizedDescription)")
+ }
+ })


Optional refactor: use a trailing closure

Auth.auth().signInAnonymously { authResult, error in if let error = error { print("Failed to sign in: \(error.localizedDescription)") } }

ncooke3 · 2021-04-15T17:08:57Z

storage/StorageExampleSwiftUI/StorageExampleSwiftUIApp.swift

+struct StorageExampleSwiftUIApp: App {
+ init() {
+ FirebaseApp.configure()
+ if Auth.auth().currentUser == nil {


Not suggesting a change, just making a note here.

This might be a good example to one day update with an Auth+Combine solution. Like instead of having this logic here, subscribing to an auth publisher and handing the app's evolving login states in some coordinator object that subscribes to the auth publisher.

Does this sound like a good use case for the Auth+Combine work that's underway? @peterfriese

ncooke3 · 2021-04-15T17:17:23Z

storage/StorageExampleSwiftUI/ImageStore.swift

+ }()
+
+ public init(withStorage storage: Storage) {
+ self.storage = storage


nit: I think Swift's design guidelines prefer to avoid having init methods that form a phrase. From here

So instead, something like:

public init(storage: Storage) { ... }

ncooke3 · 2021-04-15T17:20:36Z

storage/StorageExampleSwiftUI/ImageView.swift

+ }
+
+ func downloadImage() {
+ UserDefaults.standard.synchronize()


I think synchronize() might not need to get called at all these days. See here

Interesting. I will fix that.

ncooke3 · 2021-04-15T17:25:39Z

storage/StorageExampleSwiftUI/ImageView.swift

+ guard let imagePath = UserDefaults.standard.string(forKey: self.photoStore.imagePathKey) else { return }
+ self.photoStore.downloadImage(atPath: imagePath)


optional refactor:

if let imagePath = UserDefaults.standard.string(forKey: self.photoStore.imagePathKey) { self.photoStore.downloadImage(atPath: imagePath) }

Suggesting because it looks like we only do one thing after the guard statement and the style script might make line 69 look odd if it truncates it for being over the maximum char length

ncooke3 · 2021-04-15T19:44:46Z

storage/StorageExampleSwiftUI/ImagePicker.swift

+ if self.source == .camera {
+ imagePicker.sourceType = .camera
+ imagePicker.cameraCaptureMode = .photo
+ } else {
+ imagePicker.sourceType = .photoLibrary
+ }


optional: Since source is an enum, could do a switch case here

ncooke3 · 2021-04-15T20:02:22Z

storage/StorageExampleSwiftUI/ImagePicker.swift

+ }
+}
+
+extension ImagePickerRepresentable.Coordinator: UIImagePickerControllerDelegate {


If you haven't considered it, check out PHPickerViewController as well. There's a cool WWDC 2020 video where I learned about it

ive read a few cool articles about it. like this this one

Hey @ncooke3, I really wanted to use it too haha. I started with the PHPickerViewController. I got it working but I found two issues:

PHPickerViewController does not provide any access to the camera yet, so I was forced to also incorporate UIImagePickerController or get a camera manually.

There is a bug I found when using search in the PHPickerViewController and SwiftUI on an actual device. On my iPhone 12 Pro Max, the picker view process crashes whenever using the search bar. The rest of the controller behaves as usual. I opened feedback with Apple to address it.

I didnt know that but that's good to know! 👍

ncooke3 · 2021-04-15T20:03:43Z

storage/StorageExampleSwiftUI/ImageView.swift

+ .navigationTitle("Firebase Storage")
+ .toolbar {
+ ToolbarItemGroup(placement: .bottomBar) {
+ if !showUploadMenu {


nit: maybe refactor so it's:

if showUploadMenu { // ... } else { // !showUploadMenu related logic }

ncooke3 · 2021-04-15T20:13:48Z

storage/StorageExampleSwiftUI/ImageView.swift

+ Button("❌") {
+ showUploadMenu = false
+ }


You can use sfsymbols in conjunction with the Button API to make some really clean looking buttons

Button(action: { showUploadMenu = false }, label: { Image(systemName: "xmark.circle.fill") })

And side note, maybe remove this button entirely and instead have a "Done" button in the top right of the photo picker. I think might be a system button that you can enable (i think this functionality provided somewhere for free.. happy to help). It would then look like:

@ncooke3 Absolutely. This was a temporary fix for the selector. I was hoping to incorporate something a little more exciting and maybe some animation here.

ncooke3 · 2021-04-15T20:21:54Z

storage/StorageExampleSwiftUI/ImagePicker.swift

+import SwiftUI
+import Firebase
+
+struct ImagePickerRepresentable {


nice job with what you have going on this file. 👏 hopefully Apple provides us with a standard PhotoPicker view that saves us from having to do all of this 🤞

Thank you. Maybe there will be something announced in June. It really seems like SwiftUI was a second class citizen for the new photo APIs, so I expect them to fix that.

codeblooded force-pushed the update-storage-for-swiftui branch from ac7f271 to d818aa3 Compare March 26, 2021 20:34

paulb777 reviewed Mar 29, 2021

View reviewed changes

ncooke3 reviewed Apr 15, 2021

View reviewed changes

codeblooded mentioned this pull request Apr 19, 2021

Migrate QuickStarts to modern swift projects #1117

Open

5 tasks

codeblooded force-pushed the update-storage-for-swiftui branch from d818aa3 to b3a1d88 Compare April 20, 2021 09:27

WIP Add SwiftUI Quickstart for Storage

e05a887

codeblooded force-pushed the update-storage-for-swiftui branch from b3a1d88 to e05a887 Compare April 21, 2021 06:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Add SwiftUI Quickstart for Storage #1134

WIP Add SwiftUI Quickstart for Storage #1134

codeblooded commented Mar 26, 2021 •

edited by judyliu14

paulb777 left a comment

paulb777 Mar 29, 2021

paulb777 Mar 29, 2021

paulb777 Mar 29, 2021

ncooke3 left a comment •

edited

ncooke3 Apr 15, 2021 •

edited

ncooke3 Apr 15, 2021

ncooke3 Apr 15, 2021

ncooke3 Apr 15, 2021

codeblooded Apr 19, 2021

ncooke3 Apr 15, 2021

ncooke3 Apr 15, 2021

ncooke3 Apr 15, 2021

ncooke3 Apr 15, 2021

codeblooded Apr 19, 2021

ncooke3 Apr 19, 2021

ncooke3 Apr 15, 2021

ncooke3 Apr 15, 2021

ncooke3 Apr 15, 2021

codeblooded Apr 19, 2021

ncooke3 Apr 15, 2021

codeblooded Apr 19, 2021

		@@ -11,6 +11,8 @@
		1073487D20333C27004A66D1 /* StorageExampleUITests.m in Sources / = {isa = PBXBuildFile; fileRef = 1073487C20333C27004A66D1 / StorageExampleUITests.m */; };

		guard let imagePath = UserDefaults.standard.string(forKey: self.photoStore.imagePathKey) else { return }
		self.photoStore.downloadImage(atPath: imagePath)

WIP Add SwiftUI Quickstart for Storage #1134

Are you sure you want to change the base?

WIP Add SwiftUI Quickstart for Storage #1134

Conversation

codeblooded commented Mar 26, 2021 • edited by judyliu14

To Do

paulb777 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncooke3 left a comment • edited

Choose a reason for hiding this comment

ncooke3 Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeblooded commented Mar 26, 2021 •

edited by judyliu14

ncooke3 left a comment •

edited

ncooke3 Apr 15, 2021 •

edited