-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
50 medium widget #334
base: dev
Are you sure you want to change the base?
50 medium widget #334
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey! I haven't looked at the whole thing, but just a few questions
@@ -46,7 +46,7 @@ class AppDelegate: NSObject, UIApplicationDelegate { | |||
|
|||
FirebaseApp.configure() | |||
|
|||
let useEmulator = UserDefaults.standard.bool(forKey: "useEmulator") | |||
let useEmulator = UserDefaults.shared.bool(forKey: "useEmulator") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do? changing the UserDefaults
to use the shared app group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this so the widget would respect the same "useEmulator" value as the main app.
Basic-Car-Maintenance/Shared/Settings/ViewModels/AuthenticationViewModel.swift
Outdated
Show resolved
Hide resolved
ED42FB492CB3621B00C5CC56 /* PBXFileSystemSynchronizedBuildFileExceptionSet */ = { | ||
isa = PBXFileSystemSynchronizedBuildFileExceptionSet; | ||
membershipExceptions = ( | ||
"Shared/GoogleService-Info.plist", | ||
Shared/Models/MaintenanceEvent.swift, | ||
Shared/Models/Vehicle.swift, | ||
"Shared/UserDefaults/UserDefaults+Shared.swift", | ||
Shared/Utilities/Constants.swift, | ||
); | ||
target = FFDADF7C2ACD35A100DDEF79 /* Basic-Car-Maintenance-WidgetExtension */; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this adding Firebase to the widgets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't feel like the right way to do this....? but I'm not sure....also asking a friend who knows Firebase pretty well what they think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Thanks for looking into this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
I reviewed a few files of the PR, because this is a bigger PR. I wanted to clarify a few things, before I review the whole thing.
</array> | ||
<key>keychain-access-groups</key> | ||
<array> | ||
<string>$(AppIdentifierPrefix)com.mikaelacaron.Shared</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this entitlement do? also should this be .Shared
? as opposed to matching the bundle ID with the app name
and should there be a .
between $(AppIdentifierPrefix)
and com.mikaelacaron.Shared
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .
is included in $(AppIdentifierPrefix)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change the Shared
portion. It was a standard that I saw other devs using online.
@@ -2,6 +2,8 @@ | |||
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | |||
<plist version="1.0"> | |||
<dict> | |||
<key>KeychainAccessGroup</key> | |||
<string>$(AppIdentifierPrefix)com.mikaelacaron.Shared</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question herea bout the format (as the widget extension question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .
is included in $(AppIdentifierPrefix)
.
@@ -7,6 +7,9 @@ | |||
objects = { | |||
|
|||
/* Begin PBXBuildFile section */ | |||
ED1FE7692CB4BEDC006F861F /* FirebaseFirestoreSwift in Frameworks */ = {isa = PBXBuildFile; productRef = ED1FE7682CB4BEDC006F861F /* FirebaseFirestoreSwift */; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need to import all the Firebase stuff into the widget? Or do we not need to now, and only use keychain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since the widget uses the models/enums to fetch data from Firestore. The keychain was needed to share the authentication between applications. Firebase Cross-Application Authentication
FirebaseCore
is used to configure the firebase app.
FirebaseAuth
is used to access the current user from Auth.auth()
.
FirebaseFirestoreSwift
is needed since we are including Vehicle.swift
in the widget and that file uses a Firebase property wrapper.
<string>$(AppIdentifierPrefix)com.mikaelacaron.Shared</string> | ||
</array> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. The .
is included with the app identifier prefix.
var keychainAccessGroup: String? { | ||
return infoDictionary?["KeychainAccessGroup"] as? String | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this have a default value if nothing is found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should. I wasn't sure if we should hardcode the team name into the application.
The default value would probably need to be team-id.bundleId
What it Does
How I Tested
Notes
FirebaseAuth
, but I couldn't get anything to work.Screenshots
I would love to hear some feedback and ideas on the data sharing.