Skip to content

Conversation

caitlynjin
Copy link
Contributor

@caitlynjin caitlynjin commented Feb 25, 2024

Overview

Replaced the displayed building hours to reflect the status and open/close hours of the fitness centers. This change is applied to the gym cards on the home page as well as the gym detail pages.

Changes Made

Gym

  • Created fitnessCenterIsOpen to determine whether at least one fitness center is open at the specified gym.
  • Created determineStatus to retrieve the status of the Gym depending on the hours of its fitness centers. This status reflects the latest close time if at least one fitness center is open and the earliest open time if no fitness centers are open.

Home Gym Cell

  • Modified status text to reflect whether there is a fitness center open at the gym.
    • If a fitness center is open, the “closes at” label displays the latest close time of the fitness centers open at the gym.
    • If all fitness centers are closed, the “opens at” label displayed the earliest open time of the fitness centers at the gym.

Gym Detail View

  • Commented out code involving the building hours status and hours button.
  • Changed the displayed gym status to reflect whether at least one fitness center is currently open.

Other Changes

  • Created dummy data for Teagle under DummyData.swift. Due to missing property isSpecial , fixed previous dummy data that includes OpenHours .

Test Coverage

  • Created test cases for fitnessCenterIsOpen in Gym.
  • Created test cases for determineStatus in Gym.
  • Both of these test cases are to test the logic for varying fitness center hours at Teagle.

Next Steps

  • Determine where to display building hours.

Screenshots (optional)

Building Hours -> Gym/Fitness Center Hours
Before After
fix-hours-before.mp4
fix-hours-after.mp4

Copy link
Contributor

@vinnie4k vinnie4k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job Caitlyn! Please address my comments and request me again.

- Renamed home page labels
- Removed `GymDetailViewModel`
- Added `fitnessCenterIsOpen()` and `determineStatus()` to `Gym` model
- Created test cases for the above functions
- Created dummy data for Teagle
- Simplified status logic in `HomeGymCell`
@caitlynjin caitlynjin requested a review from vinnie4k March 6, 2024 19:12
Copy link
Contributor

@vinnie4k vinnie4k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job on addressing my comments Caitlyn! Essentially good to go, but there are just a few typos and quick changes you should address before merging. Hopefully you understand how unit tests work and how useful they are!

In the future, we should expand our testing and create multiple test suites. Because we only test a few functions, this is fine for now. We should also configure a GitHub workflow that triggers an Xcode command line to run the test suite on PR creation, but it's not that important.

- Fixed grammatical errors
Copy link
Contributor

@belle-hu belle-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really cool! I'm going to reference this in the future when implementing logic! Looks good btw.

Comment on lines +135 to +149
// TODO: Removed building hours. Determine what should be displayed here.
// switch gym.status {
// case .closed:
// Text("CLOSED")
// .font(Constants.Fonts.h3)
// .foregroundStyle(Constants.Colors.closed)
// case .open:
// Text("OPEN")
// .font(Constants.Fonts.h3)
// .foregroundStyle(Constants.Colors.open)
// case .none:
// EmptyView()
// }
//
// viewHoursButton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably need to delete this for style right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're holding onto this code for now because we had to remove building hours, not sure what we're going to do with it yet.

@caitlynjin caitlynjin merged commit 890d3f9 into main Mar 12, 2024
@caitlynjin caitlynjin deleted the caitlyn/fix-hours branch March 12, 2024 04:04
@caitlynjin caitlynjin mentioned this pull request Mar 12, 2024
This was referenced Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants