Ximluo/gsr sharing merge #614
Conversation
anli5005
left a comment
anli5005
left a comment
There was a problem hiding this comment.
Looking good so far! Main blocker is adding a way to access the share screen from the new reservations tab - I think you were working on that before we redid most of the GSR flow.
Other than that, some comments:
| location: String, | ||
| start: Date, | ||
| end: Date, | ||
| completion: @escaping (Bool) -> Void |
There was a problem hiding this comment.
nit: Would prefer we made this async
| completion: @escaping (Bool) -> Void | ||
| ) { | ||
| let eventStore = EKEventStore() | ||
| eventStore.requestAccess(to: .event) { granted, error in |
There was a problem hiding this comment.
Should we use something like requestWriteOnlyAccessToEvents() instead?
Alternatively, there's a way to present an EKEventEditViewController without asking for permission - it probably requires more effort on the user's part, but might be worth looking into this.
|
|
||
| /// Observed by SwiftUI to detect new deep links. | ||
| class DeepLinkManager: ObservableObject { | ||
| @Published var lastResolvedLink: GSRShareModel? |
There was a problem hiding this comment.
It would probably be best to migrate this to some sort of enum so we can support multiple shared links, but that's a simple change in the future so won't block on this
| fatalError("Unhandled auth manager state: \(authManager.state)") | ||
| } | ||
| } | ||
| .onAppear { |
There was a problem hiding this comment.
Should we make this a .onChange on deepLinkManager.lastResolvedLink instead?
| } | ||
| .store(in: &cancellables) | ||
| } | ||
| .sheet(isPresented: $showShareDetail) { |
There was a problem hiding this comment.
It might be cleaner if we use the other variant of the .sheet modifier so we can combine currentShare and showShareDetail into one state variable.
| <key>CFBundleURLName</key> | ||
| <string></string> | ||
| <string>org.pennlabs.PennMobile.dev</string> | ||
| <key>CFBundleURLSchemes</key> | ||
| <array> | ||
| <string>https</string> | ||
| </array> | ||
| <key>CFBundleURLTypes</key> | ||
| <array> | ||
| <dict> | ||
| <key>CFBundleURLName</key> | ||
| <string></string> | ||
| </dict> | ||
| </array> | ||
| </dict> | ||
| <dict> | ||
| <key>CFBundleTypeRole</key> | ||
| <string>Editor</string> | ||
| <key>CFBundleURLSchemes</key> | ||
| <array/> | ||
| <array> | ||
| <string>https</string> | ||
| </array> |
There was a problem hiding this comment.
Is this a requirement for universal link support?
| <key>NSLocationWhenInUseUsageDescription</key> | ||
| <string>Enable to show your current location on the map</string> |
There was a problem hiding this comment.
Was there a reason we removed this?
There was a problem hiding this comment.
Do we need to add some sort of purpose string for calendar access? Not sure
No description provided.