Add ecosystem gym UI and refactored common interface for ecosystem models#121
Add ecosystem gym UI and refactored common interface for ecosystem models#121
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds gym functionality to the ecosystem bottom sheet and refactors the codebase to use a common interface pattern for ecosystem models (eateries, gyms, libraries, and printers). The refactoring extracts the operating hours formatting and place conversion logic into shared interfaces.
Changes:
- Introduced
EcosystemPlaceandDetailedEcosystemPlaceinterfaces to standardize place conversion and operating hours handling across eateries, gyms, libraries, and printers - Added comprehensive gym UI including
GymDetailsContentandGymCapacityIndicatorcomponents - Moved shared utilities (day ordering, capacity thresholds, color interpolation) to common utility files
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
app/src/main/res/values/strings.xml |
Added "Learn More on Uplift" string resource for gym details |
app/src/main/java/com/cornellappdev/transit/util/ecosystem/PlaceUtils.kt |
Deleted - functionality moved to interface implementations |
app/src/main/java/com/cornellappdev/transit/util/TransitConstants.kt |
Added capacity threshold constants for gym indicators |
app/src/main/java/com/cornellappdev/transit/util/TimeUtils.kt |
Added shared day ordering maps for operating hours |
app/src/main/java/com/cornellappdev/transit/util/ContentConstants.kt |
Added gym location and about content mappings |
app/src/main/java/com/cornellappdev/transit/util/ColorUtils.kt |
Added color interpolation utilities from Uplift Android |
app/src/main/java/com/cornellappdev/transit/ui/viewmodels/HomeViewModel.kt |
Added capacity formatting and made getOpenStatus public; added internal rotation to getOpenStatus |
app/src/main/java/com/cornellappdev/transit/ui/viewmodels/FavoritesViewModel.kt |
Removed unused import after PlaceUtils deletion |
app/src/main/java/com/cornellappdev/transit/ui/theme/Color.kt |
Added gym-specific colors (AccentOpen, AccentClosed, AccentOrange) |
app/src/main/java/com/cornellappdev/transit/ui/components/home/RoundedImagePlaceCard.kt |
Removed padding modifier (now controlled by parent LazyColumn) |
app/src/main/java/com/cornellappdev/transit/ui/components/home/OperatingHoursList.kt |
Adjusted column weights for better layout balance |
app/src/main/java/com/cornellappdev/transit/ui/components/home/GymDetailsContent.kt |
New file - implements full detail view for gyms |
app/src/main/java/com/cornellappdev/transit/ui/components/home/GymCapacityIndicator.kt |
New file - circular progress indicator for gym capacity |
app/src/main/java/com/cornellappdev/transit/ui/components/home/EcosystemBottomSheetContent.kt |
Updated gym list to display full cards with favorites, operating hours, and capacity |
app/src/main/java/com/cornellappdev/transit/ui/components/home/EateryDetailsContent.kt |
Updated to use interface method operatingHours() instead of formatOperatingHours() |
app/src/main/java/com/cornellappdev/transit/ui/components/home/DetailedPlaceSheetContent.kt |
Added gym details handling and navigation |
app/src/main/java/com/cornellappdev/transit/ui/components/home/DetailedPlaceHeaderSection.kt |
Added variant with widget support for gym capacity indicator |
app/src/main/java/com/cornellappdev/transit/models/ecosystem/UpliftGym.kt |
Implemented DetailedEcosystemPlace interface with operating hours conversion |
app/src/main/java/com/cornellappdev/transit/models/ecosystem/StaticPlaces.kt |
Implemented EcosystemPlace interface for Printer and DetailedEcosystemPlace for Library |
app/src/main/java/com/cornellappdev/transit/models/ecosystem/EcosystemPlace.kt |
New file - common interfaces for ecosystem places |
app/src/main/java/com/cornellappdev/transit/models/ecosystem/Eatery.kt |
Refactored to implement DetailedEcosystemPlace interface and use shared day ordering |
app/src/main/java/com/cornellappdev/transit/models/ecosystem/DetailedEcosystemPlace.kt |
Deleted - merged into EcosystemPlace.kt |
app/src/main/java/com/cornellappdev/transit/models/LocationRepository.kt |
Updated comment formatting |
Comments suppressed due to low confidence (5)
app/src/main/java/com/cornellappdev/transit/ui/components/home/GymDetailsContent.kt:117
- The function getGymLocationString returns a String, but at the call site on line 117, it's being treated as nullable (using the elvis operator ?:). Since the function always returns a non-null String (either a mapped value or an empty string), the elvis operator is unnecessary here.
text = getGymLocationString(gym.name) ?: "",
app/src/main/java/com/cornellappdev/transit/ui/components/home/GymCapacityIndicator.kt:62
- The color interpolation logic in this comment doesn't match the implementation. The comment says "If between 0 & 0.5, tween between open and orange. If between 0.5 and 1, tween between orange and closed." However, the actual HIGH_CAPACITY_THRESHOLD is 0.65, not 0.5. Additionally, the else branch (lines 71-75) interpolates between AccentOpen and AccentClosed, not between open and orange as the comment suggests.
// Choose a color. If between 0 & 0.5, tween between open and orange. If between 0.5 and 1,
// tween between orange and closed.
app/src/main/java/com/cornellappdev/transit/models/ecosystem/EcosystemPlace.kt:27
- The documentation comment for operatingHours() states it returns hours "representing each day of the week and the corresponding times that an eatery is open". This should be generalized to say "that this place is open" since this interface is now used for both eateries and gyms, not just eateries.
/**
* @Return a list of associated dayOfWeek and hours pairs in [DayOperatingHours] representing
* each day of the week and the corresponding times that an eatery is open. The list is sorted
* by day with the custom dayOrder (Sunday first).
*/
fun operatingHours(): List<DayOperatingHours>
app/src/main/java/com/cornellappdev/transit/ui/viewmodels/HomeViewModel.kt:366
- The function getOpenStatus was changed from private to public. This seems to be done to support the call in GymDetailsContent.kt line 42. However, since getOpenStatus now internally rotates the operating hours (line 367), calling it directly with gym.operatingHours() will work correctly. But the function's documentation should be updated to clarify that it expects non-rotated operating hours as input since it handles rotation internally.
/**
* Given operating hours, return whether it is open and when it is open until
* or when it will next open
*
* @param operatingHours A list of pairs mapping the first value day string to second value list of hours open
*/
fun getOpenStatus(
operatingHours: List<DayOperatingHours>,
currentDateTime: LocalDateTime = LocalDateTime.now()
): OpenStatus {
app/src/main/java/com/cornellappdev/transit/util/ContentConstants.kt:3
- This import statement appears to be unused. The file does not use any functionality from android.adservices.adid.AdId. This import should be removed to keep the code clean.
import android.adservices.adid.AdId
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/src/main/java/com/cornellappdev/transit/ui/viewmodels/HomeViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (7)
app/src/main/java/com/cornellappdev/transit/ui/components/home/DetailedPlaceHeaderSection.kt:15
- Unused import
Alignment.Companion.TopEndshould be removed.
import androidx.compose.ui.Alignment.Companion.TopEnd
app/src/main/java/com/cornellappdev/transit/util/ContentConstants.kt:3
- Unused import
android.adservices.adid.AdIdshould be removed to avoid warnings/lint failures (and it’s not available on all API levels if it ever becomes used).
import android.adservices.adid.AdId
app/src/main/java/com/cornellappdev/transit/util/ContentConstants.kt:45
getGymLocationStringrebuilds a new map on every call; this is invoked while rendering lists (e.g., bottom sheet gyms) and will allocate repeatedly on recomposition. Consider making the map aprivate val(or using awhen) and just doing a lookup here.
fun getGymLocationString(key: String): String {
val locationStrings = buildMap {
put("Helen Newman", "Helen Newman Hall")
put("Toni Morrison", "Toni Morrison Hall")
put("Noyes", "Noyes Community Recreation Center")
put("Teagle Up", "Teagle Hall")
put("Teagle Down", "Teagle Hall")
}
return locationStrings.getOrDefault(key, "")
}
app/src/main/java/com/cornellappdev/transit/ui/components/home/GymCapacityIndicator.kt:59
LaunchedEffect(animatedFraction)will only run once because the key never changes; if capacity updates while the screen is visible, the progress won’t animate (or even update) to the new value. Use a key that changes with the target fraction (e.g., the percent/fraction) so updates re-trigger the animation.
// When the composable launches, animate the fraction to the capacity fraction.
LaunchedEffect(animatedFraction) {
animatedFraction.animateTo(
fraction,
animationSpec = tween(durationMillis = 750)
)
}
app/src/main/java/com/cornellappdev/transit/ui/components/home/DetailedPlaceHeaderSection.kt:10
- Unused import
androidx.compose.foundation.layout.sizeshould be removed.
import androidx.compose.foundation.layout.size
app/src/main/java/com/cornellappdev/transit/models/ecosystem/StaticPlaces.kt:51
Library.operatingHours()is currently a TODO returningemptyList(), which violates theDetailedEcosystemPlacecontract (and will crash callers that assume at least 7 entries, likegetOpenStatuswhich indexes[0]). Consider returning a 7-day list of "Closed" values until real hours are available, or avoid implementingDetailedEcosystemPlaceforLibraryuntil hours are supported.
override fun operatingHours(): List<DayOperatingHours> {
//TODO: Implement
return emptyList()
}
app/src/main/java/com/cornellappdev/transit/ui/components/home/GymCapacityIndicator.kt:75
- The low-capacity interpolation uses
AccentOpen→AccentClosed, which skipsAccentOrangeentirely (despite the comment and the medium threshold concept elsewhere). If the intention is green→orange→red, the first segment should interpolate toward orange (and then orange→red for the high segment).
else
colorInterp(
fraction / HIGH_CAPACITY_THRESHOLD,
AccentOpen,
AccentClosed
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val color = if (capacity.percent <= MEDIUM_CAPACITY_THRESHOLD) { | ||
| AccentOpen | ||
| } else if (capacity.percent >= HIGH_CAPACITY_THRESHOLD) { |
There was a problem hiding this comment.
capacity.percent is a Double but the capacity thresholds are Floats, so these comparisons won’t compile without explicit conversion. Align the numeric types (e.g., make thresholds Double or convert percent to Float) so this function builds.
| val color = if (capacity.percent <= MEDIUM_CAPACITY_THRESHOLD) { | |
| AccentOpen | |
| } else if (capacity.percent >= HIGH_CAPACITY_THRESHOLD) { | |
| val percent = capacity.percent.toFloat() | |
| val color = if (percent <= MEDIUM_CAPACITY_THRESHOLD) { | |
| AccentOpen | |
| } else if (percent >= HIGH_CAPACITY_THRESHOLD) { |
| // Choose a color. If between 0 & 0.5, tween between open and orange. If between 0.5 and 1, | ||
| // tween between orange and closed. | ||
| val color = | ||
| if (fraction > HIGH_CAPACITY_THRESHOLD) | ||
| colorInterp( | ||
| (fraction - HIGH_CAPACITY_THRESHOLD) / (1 - HIGH_CAPACITY_THRESHOLD), | ||
| AccentOrange, | ||
| AccentClosed | ||
| ) |
There was a problem hiding this comment.
The color interpolation around HIGH_CAPACITY_THRESHOLD is discontinuous: at exactly the threshold the else branch produces the end color (closed/red), but just above the threshold the if branch starts at orange. This causes a visible jump and effectively flips colors right after crossing the threshold. Adjust the branching/interpolation so the color changes smoothly and matches the intended green→orange→red behavior.
| const val MEDIUM_CAPACITY_THRESHOLD = 0.35f | ||
|
|
||
| /** When the capacity turns to red */ | ||
| const val HIGH_CAPACITY_THRESHOLD = 0.65f |
There was a problem hiding this comment.
MEDIUM_CAPACITY_THRESHOLD/HIGH_CAPACITY_THRESHOLD are Floats but capacity percent is modeled as Double (UpliftCapacity.percent). This will force callers to do explicit conversions and currently causes type mismatch in comparisons. Consider making these thresholds Double (or changing percent to Float) for consistent numeric types.
| const val MEDIUM_CAPACITY_THRESHOLD = 0.35f | |
| /** When the capacity turns to red */ | |
| const val HIGH_CAPACITY_THRESHOLD = 0.65f | |
| const val MEDIUM_CAPACITY_THRESHOLD = 0.35 | |
| /** When the capacity turns to red */ | |
| const val HIGH_CAPACITY_THRESHOLD = 0.65 |
| * Text area of detailed place header with favorites star on the top right and widget on the bottom right | ||
| */ | ||
| @Composable | ||
| fun DetailedPlaceHeaderSectionWithWidget( |
There was a problem hiding this comment.
This looks pretty similar to the above DetailedPlaceHeaderSection composable in implementation, is there any way we can consolidate them or share ui code between them?
|
|
||
| @Preview(showBackground = true) | ||
| @Composable | ||
| private fun DetailedPlaceSheetContentPreview() { |
There was a problem hiding this comment.
Nit: Would probably be nice to have something like preview parameters for the different types of sheet content (eg. gym, eatery, etc.)
| }, | ||
| ) | ||
|
|
||
| Spacer(modifier = Modifier.height(24.dp)) |
There was a problem hiding this comment.
Nit: Could potentially just switch out spacers with verticalArrangement spacedby and grouping certain composables together
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding( | ||
| start = 20.dp, |
There was a problem hiding this comment.
Nit: can just use horizontal = 20.dp
| */ | ||
| @Composable | ||
| fun GymDetailsContent( | ||
| homeViewModel: HomeViewModel = hiltViewModel(), |
There was a problem hiding this comment.
I think we probably shouldn't pass homeviewmodel here directly and just pass the functions that are needed
| * Displays the full detail view for an individual gym within the ecosystem bottom sheet. | ||
| */ | ||
| @Composable | ||
| fun GymDetailsContent( |
There was a problem hiding this comment.
Nit: would probably be nice to have a preview function
| modifier = Modifier.padding(bottom = 15.dp) | ||
| ) | ||
|
|
||
| Text( |
There was a problem hiding this comment.
I think it's missing the icon + arrow. I think it's fine to skip, but should probably make note somewhere for future implementations assuming the intent is to be able to launch app from this one.
| shouldClipBottom = true | ||
| ) | ||
|
|
||
| DetailedPlaceHeaderSectionWithWidget( |
There was a problem hiding this comment.
I think this is missing the distance from current user in mi. If this is out of scope for current PR, should probably make note somewhere
| * @param operatingHours A list of pairs mapping the first value day string to second value list of hours open | ||
| */ | ||
| private fun getOpenStatus( | ||
| fun getOpenStatus( |
There was a problem hiding this comment.
Nit: For this and the below functions, if they don't affect the home ui state in any way, they can potentially be extracted into some other file as util functions separate from the vm
| onDetailsClick: (DetailedEcosystemPlace) -> Unit, | ||
| favorites: Set<Place>, | ||
| onFavoriteStarClick: (Place) -> Unit, | ||
| operatingHoursToString: (List<DayOperatingHours>) -> AnnotatedString, |
There was a problem hiding this comment.
Nit: for this and the below function, if they are extracted from vm as separate util functions, can probably remove the param and import directly
Overview
Added gyms to ecosystem bottom sheet and refactored so that they share common functions
Changes Made
Test Coverage
Tested on Pixel 3a
Next Steps (delete if not applicable)
Related PRs or Issues (delete if not applicable)
Screenshots (delete if not applicable)
Card Screenshot
Content Screenshot
Video
gym.webm