Updated UI and singleton services #45
Conversation
tbeidlershenk
added
enhancement
|
Pull request not being accepted at the moment. There seems to be some functionality issues at the moment. (Testing on dev db) My observations:
My suggestion now is, are we able to take one push at time and test? That way we can pinpoint single issues. Right now there's too many things causing errors that I would like to take it one adjustment at a time. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lib/pages/item_request.dart
Outdated
| Item item = Item(id: "", userId: AuthService.instance.user!.uid, timeCreated: Timestamp.now(), description: description, itemName: category, location: GeoPoint(lat, long),phone: phone, picture: _imgName); | ||
| DocumentReference ref = await FirebaseFirestore.instance.collection('lost').add(item.toFirestore()); | ||
| item.id = ref.id; | ||
| DataService.instance.lostItems.add(item); |
There was a problem hiding this comment.
There should be a better way to manage lost and found items. We stream from FirebaseFirestore for a reason, it allows us to sync with changes in the backend. There should not be a need to add via locally. We can instead stream the collection and update any info needed to be displayed accordingly.
There was a problem hiding this comment.
Updated the methods to not add locally. Doesn't update automatically but it does if you refresh
| return StreamBuilder<MyUser?>( | ||
| stream: authService.user, | ||
| builder: (_, AsyncSnapshot<MyUser?> snapshot) { | ||
| return StreamBuilder<User?>( |
There was a problem hiding this comment.
You got rid of the provider here. Just wondering why, I had provider here so we can access user info anywhere in app.
There was a problem hiding this comment.
The way I have it we still can access it anywhere. We can go back to using the Provider, but it works the same using the singleton instance pattern:
<ServiceClassName>.instance.<whatever you need to access> should work from any file
There was a problem hiding this comment.
^ followup to this is there an advantage of using Provider class instead? I haven't looked into it at all
| /** | ||
| * Singleton service for making Firebase calls | ||
| */ | ||
| class FirebaseService { |
There was a problem hiding this comment.
This seems wrong for some reason. Firebase itself is already a class and has methods in it, feels wrong to package it up again? Feel free to correct me if I'm wrong. Also what was with it being in the auth.dart file?
There was a problem hiding this comment.
Mostly just for the convenience of calling one method to do something. Take createUser as an example. It takes the AppUser datamodel and then performs the database call that creates the firebase mapping. I'm not opposed to moving these method(s) though to the AuthService class instead, that might make more sense - because i just realized that we have a service class calling a service class which is honestly probably not good.
There was a problem hiding this comment.
Your call on what to do about this, let me know, I can refactor it
| lastName: lastName, | ||
| location: defaultGeopoint); | ||
| this.user = user; | ||
| await FirebaseService.instance.createUser(user); |
There was a problem hiding this comment.
What is going on with line 41-43?
There was a problem hiding this comment.
line 42 creates a new user document in Firestore. line 41 appears to be a mistake since this is in the register method not the sign in method. But there's a similar line to 41 in signInWithEmailAndPassword that keeps the AppUser instance of AuthService consistent with the sign in state held in FirebaseAuth. Is there a way we can just get this to automatically update by listening to the FirebaseAuth stream? This would be a better way
Stream<User?> get authUser => FirebaseAuth.instance.authStateChanges();
^ this one in AuthService
|
Appreciate all the work you're putting in @tbeidlershenk👏. Observations:
|
|
Closing as we won't be merging this but will still be referencing it |
Changes
AuthServiceby making it singleton and to correctly update Firestore with user documentsAppUserto match a user document in FirestoreFirebaseServicewhere we should put all related Firestore callsDataServicethat stores the current lost and found items so we can access from anywhere in the appgoogle_nav_barpackage in a newHomePageclass that manages all routes in the app, both existing and to be implemented (chat, profile page) and moved theStreamBuilderlistening to this page fromMapPageroute.dartand other files as neededNotes
Too many changes bruh please pull and test this Jeff!!!
One note I wasn't able to get it to auto update the map when submitting a post which is weird because I think the StreamBuilder should pick up on this but idrk. It does refresh if you physically refresh the page by navigating away then back.