Luizmello/contacts app #1
Conversation
There was a problem hiding this comment.
Overall, it does function as the directions say but there are many areas that need improvement. Specifically, around aesthetics and and simplification. Also, I know you were trying to go above and beyond but taking our image and manipulating it it's appropriate because the splash image (LaunchScreenLogo) doesn't look correct. The golden color is missing within the B. So if it's not correct, we shouldn't use it. 
If you read through my comments, you'll see examples of what I had mentioned before in the email about simplifying and cleaning up code. I looked through each file but stopped commenting thoroughly because I realize this is part of a larger discussion we should have. But I wanted to leave some comments you could see some of my feedback. There's a lot that could/should be further simplified and reduced, as well as clarifying naming and overall structure. Those things are not an exact science so they will probably take more time to develop.
But, this is part of the learning process so don't take any of the comments personally. There's many positives too but when we submit code for review, mostly only the negatives get mentioned unfortunately.
| let thumbnail: String? | ||
| } | ||
|
|
||
| struct coordinates: Equatable, Decodable{ |
There was a problem hiding this comment.
We should always capitalize objects and keep spacing appropriate.
| struct coordinates: Equatable, Decodable{ | |
| struct Coordinates: Equatable, Decodable { |
| let last: String? | ||
| } | ||
|
|
||
| struct Picture: Equatable, Decodable{ |
There was a problem hiding this comment.
Needs spacing
| struct Picture: Equatable, Decodable{ | |
| struct Picture: Equatable, Decodable { |
| let longitude: String? | ||
| } | ||
|
|
||
| struct location: Equatable, Decodable{ |
There was a problem hiding this comment.
Capitalization and spacing
| struct location: Equatable, Decodable{ | |
| struct Location: Equatable, Decodable { |
| let coordinates: coordinates? | ||
| } | ||
|
|
||
| struct street: Equatable, Decodable{ |
There was a problem hiding this comment.
Capitalization and spacing
| struct street: Equatable, Decodable{ | |
| struct Street: Equatable, Decodable { |
| let picture: Picture | ||
| let email: String | ||
| let phone: String | ||
| let location: location |
There was a problem hiding this comment.
| let location: location | |
| let location: Location |
| @State var user: Person | ||
| var body: some View { | ||
| VStack{ | ||
| MapView(lat: user.location.coordinates?.latitude, long: user.location.coordinates?.latitude) |
There was a problem hiding this comment.
The maps don't display properly. They seem to not know what to display as the view seems to be in the middle of nowhere.
| var body: some View { | ||
| AsyncImage(url: URL(string: userImage)) | ||
| .aspectRatio(contentMode: .fit) | ||
| .frame(width: 128, height: 128) |
There was a problem hiding this comment.
To make this reusable, we could remove this so the view that implements it could specify it.
| .aspectRatio(contentMode: .fit) | ||
| .frame(width: 128, height: 128) | ||
| .clipShape(Circle()) | ||
| .overlay(Circle().stroke(Color.gray, lineWidth: 2)) |
There was a problem hiding this comment.
Aesthetically, having the extra circle stroke is a bit confusing.
| import SwiftUI | ||
| import MapKit | ||
|
|
||
| struct MapView: UIViewRepresentable { |
There was a problem hiding this comment.
SwiftUI has a Map object which would be better to use and customize than creating a representable unless we need some specific functionality that the native objects won't give us.
|
|
||
| import SwiftUI | ||
|
|
||
| struct TestAsync: View { |
There was a problem hiding this comment.
This isn't used anywhere in the code.
Summary
I created an app thats makes an API request presenting a list of users and a user detail navigation. Then I tried removing code smells and simplifying the code. I wasn't able to remove an specific error in the console. The APII has several data (users) coming with different "documentation" (For example, in one user there are
results{location {street{name: Street Name}}}, but in others there isn't the street object, it's directly inside the location, which is causing some errors but doesn't affect the application's performance.I didn't do unit tests for lack of time and for worrying more about turning fetch into async/await.
Items Completed
Images/Videos