-
Notifications
You must be signed in to change notification settings - Fork 0
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
Code Refactoring v0.0.1 #16
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,5 @@ | |||
|
|||
iStore.xcodeproj/project.xcworkspace/xcuserdata/mohamedsayed.xcuserdatad/UserInterfaceState.xcuserstate |
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.
why did you add this change?
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.
It caused Problems with Applying stashed or merging other branches into the branch.
var body: some View { | ||
TabView { | ||
HomeView() | ||
HomeView(id: user.id, homeAddress: user.address.address, homePostalCode: user.address.postalCode, homeCity: user.address.city, homeState: user.address.state, homeLatitude: user.address.coordinates.lat, homeLongitude: user.address.coordinates.lng, currentAddress: locationDataManager.address, currentPostalCode: locationDataManager.postalCode, currentCity: locationDataManager.city, currentState: locationDataManager.state, currentLatitude: locationDataManager.latitude, currentLongitude: locationDataManager.longitude, isCurrentLocation: locationDataManager.isCurrentLocation, isGuest: isGuest, getLocationPermission: locationDataManager.requestUserLocationPermission) |
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 it better to create a custom object with all passing properties and pass it directly to the HomeView
? just an idea!
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.
ok, I will try it and push the changes.
authenticationViewModel.authenticateUser(userName: userName, password: password) { | ||
navigationCoordinator.switchView = .contentView | ||
authenticationViewModel.showProgress = false | ||
authenticationViewModel.authenticateUser() { result in |
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.
Why do you call this method in the view?
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.
Because It is the action for the login button.
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.
Can't see the button here :)
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.
you need to look better.
switch result { | ||
case .success(let currentUser): | ||
DispatchQueue.main.async { | ||
authenticationViewModel.user = currentUser |
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.
Also here why do you assign the values here?
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.
Because It's the completion of the authencation Method.
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.
There is something wrong here. This logic should be handle by the model view.
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 have to do it here as it affects the UI and the navigationCoordinator is an EnvironmentObject so It's properties must be accessed from the view.
@@ -11,7 +11,7 @@ import SwiftUI | |||
struct iStoreApp: App { | |||
var body: some Scene { | |||
WindowGroup { | |||
ViewManager(navigationCoordinator: NavigationCoordinator(), authenticationViewModel: AuthenticationViewModel(authenticationManager: AuthenticationManager(), user: AuthenticatedUser(id: Int(), username: String(),firstName: String(), lastName: String(), maidenName: String(), email: String(), phone: String(), image: URL(fileURLWithPath: String()), address: HomeAddress(address: String(), city: String(), postalCode: String(), coordinates: Coordinates(lat: Double(), lng: Double()), state: String()))), locationDataManager: LocationDataManager(address: String(), postalCode: String(), city: String(), state: String(), latitude: Double(), longitude: Double())) | |||
ViewManager(navigationCoordinator: NavigationCoordinator(user: AuthenticatedUser(id: Int(), username: String(),firstName: String(), lastName: String(), maidenName: String(), email: String(), phone: String(), image: URL(fileURLWithPath: String()), address: HomeAddress(address: String(), city: String(), postalCode: String(), coordinates: Coordinates(lat: Double(), lng: Double()), state: String()))), authenticationViewModel: AuthenticationViewModel(authenticationManager: AuthenticationManager(), user: AuthenticatedUser(id: Int(), username: String(),firstName: String(), lastName: String(), maidenName: String(), email: String(), phone: String(), image: URL(fileURLWithPath: String()), address: HomeAddress(address: String(), city: String(), postalCode: String(), coordinates: Coordinates(lat: Double(), lng: Double()), state: String())), userName: "kminchelle", password: "0lelplR"), locationDataManager: LocationDataManager(address: String(), postalCode: String(), city: String(), state: String(), latitude: Double(), longitude: Double())) |
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's ViewManager
? and Why did you pass all these parameters?
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.
It's responsible for navigating from loginView to ContentView and passing the user after authentication, It takes all these params because It holds the NavigationCoordinator, the AuthenticationViewModel and the LocationManager. And each one of them has Its own Params that need to be passed.
Requesting Code Review.