Feat/training coach frontend final #231
Conversation
…ining-coach-frontend
…ining-coach-frontend # Conflicts: # app/src/main/res/values/strings.xml
Feat/training coach debug
There was a problem hiding this comment.
Note: I did remove a lot of tests, since I refactor the whole PairingRequest
There was a problem hiding this comment.
Note: No need to review this file since it was reviewed in this PR #232
There was a problem hiding this comment.
Thank you for this Pull Request! It is a really impressive feature, that will have a significant impact on the user experience. It is really coherent with the first objective of the app which was to make athletes train together!
I have a few comment that I would like to address. There is a certain number of ressources (colors and string) that are added and unused, I am not sure if it is for a latter update so I just want to highlight it.
Concerning the feature, I could not test is a 100% as is require 2 devices. I think it would be really beneficial if another reviewer could try this feature on 2 real devices. (I think that the author as already 2 devices so it should be already tested, but since we are at the end the project it is great to double check everything.)
Concerning the UI, I have a small detail that I would like to discuss on the screenshot bellow:
(Also if there is a way to change the background color of the pop up, as it is quite far from the rest of the app, but this just really a detail)
I think that the title "Choose Your Role" and the question that follows are a bit redundant, given that then the Athlete/coach changes when we tick the box. I think that we could either remove the title or the question (Or make the question become the title perhaps?)
| val status = if (isAccepted) RequestStatus.ACCEPTED else RequestStatus.REJECTED | ||
| db.collection(PAIRING_REQUESTS).document(requestId).update("status", status.name).await() | ||
| override suspend fun updatePairingRequestStatus(requestId: String, status: RequestStatus) { | ||
| require(requestId.isNotEmpty()) { "The request ID must not be empty." } |
There was a problem hiding this comment.
Suggestion: you could define a constant for this string, as it is also used in line 473 and line 486
| import androidx.compose.foundation.layout.padding | ||
| import androidx.compose.foundation.layout.size | ||
| import androidx.compose.foundation.layout.width | ||
| import androidx.compose.foundation.lazy.LazyColumn | ||
| import androidx.compose.foundation.lazy.grid.items |
There was a problem hiding this comment.
Import: this unused import could be removed
| } | ||
|
|
||
| // Utility to fetch activity icon | ||
| @Composable |
There was a problem hiding this comment.
Improvement: We have the same function in java/com/android/streetworkapp/ui/progress/Progression.kt at the end of the file. Since this function seems to be useful in many parts of the app, we should perhaps deplace it in a utility file and use it in the different parts of the app, rather than rewriting it each time
There was a problem hiding this comment.
I merge the two functions and put this function in the utils folder
app/src/main/res/values/colors.xml
Outdated
| @@ -3,6 +3,7 @@ | |||
| <color name="purple_200">#FFBB86FC</color> | |||
| <color name="purple_500">#FF6200EE</color> | |||
There was a problem hiding this comment.
Question: I think(if I am not mistaken) that this color is unused, do you plan to use it later? If not perhaps we could remove it.
There was a problem hiding this comment.
Indeed it was a mistake first I used purple_500 then purple_200 and I forgot to remove this color
app/src/main/res/values/strings.xml
Outdated
| <string name="time_activity_in_progress">The activity is time-based and currently in progress. Your coach manage the timer.</string> | ||
| <string name="reps_activity_in_progress">The activity is repetition-based and currently in progress. Your coach manage the counter.</string> | ||
| <string name="end_session_button_text">End session</string> | ||
| <string name="counter_as_coach">Counter as coach</string> |
There was a problem hiding this comment.
Same comment as above: this string is also unused, is it to keep it for a futur improvement? (it is also the case for some other strings in this file, if you would like to check)
| modifier = Modifier.fillMaxSize().padding(16.dp), | ||
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| verticalArrangement = Arrangement.Center) { | ||
| Text("No session data available.") |
There was a problem hiding this comment.
Refactor: the string could be added to the string.xml file
…o be updated in this branch
|
Overview
This PR completes the training part of the app, with the option of training with a coach. Both users must be friends to train with a coach. The athlete chooses an activity and the coach can update the athlete's training.
The PR is large, but some of parts has already been reviewed in #232. The problem with this PR is that in order to test whether it works, you need to implement the backend and the frontend to test on two phones.
Visual aspect
Testing
The testing part doesn't reach 80% line coverage, as I haven't found an easy way to test the observables that are needed during training.