-
Notifications
You must be signed in to change notification settings - Fork 1
Duru/workout-checkin #115
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
base: main
Are you sure you want to change the base?
Duru/workout-checkin #115
Conversation
MrPeterss
left a comment
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.
Nice job, Duru! Just a couple of comments to address. I'll approve this but don't merge until the comments are address (and you get another review lol)
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.
nice!
| visibility?(false) | ||
| return | ||
| } | ||
| let gymsByDistance = gyms.sorted { g1, g2 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.
general rule of thumb. closing braces inside of functions should have newlines after them. so put a newline above this line
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 this in the whole file as well
| } | ||
| } | ||
| } | ||
| } |
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.
nice job on the logic in this file! looks really great
| var body: some View { | ||
| if !isCheckedIn { | ||
| HStack(spacing: 20) { | ||
| VStack(alignment: .leading) { |
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.
inside of Hstacks and vstacks, put a newline space between each of the children elements (in here, between the two Text elements) just for better visibility
| .font(Constants.Fonts.labelNormal) | ||
| .foregroundStyle(Constants.Colors.gray04) | ||
| } | ||
| HStack(spacing: 16) { |
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 here, newline above. im going to stop commenting on it now but don't forget to fix elsewhere too!
| @State private var selectedTab: Screen = .home | ||
| @StateObject var tabBarProp = TabBarProperty() | ||
| @StateObject private var homeViewModel = HomeView.ViewModel() | ||
| @StateObject private var profileViewModel = ProfileView.ViewModel() |
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 was sceptical about this practice but i like this use of @ObservedObject. If these are used in many different views, we might be better off making it an environemntObject, but this is ok for now
Demo: https://github.com/user-attachments/assets/f40ef770-e2ce-4c1c-b03a-6d4311e37df1
I tried my best, but there may be a lot of bad practices (for the video, just to be able to simulate used a 0.3-mile threshold):
Within a distance of 0.05 mi from an open gym, the pop-up appears
When the pop-up is closed in the proximity of a gym, there is a 2-hour cooldown until it appears again
When the user checks in, an animation of confetti gets displayed, and the user's data gets updated (pop-up disappears after 10 seconds)
Note: the update logic is kind of a dummy data logic, doesn't have the badges update, workout history can probably get improved, etc. Also doesn't involve backend since I believe the app uses dummy data for the profile view for now.