-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feat/alert/alert lists UI improvements #342
Conversation
…feat/alert/alertLists-ui-improvements
- Changed dispatchers from main to IO for tasks that should run in the background, in `UserModelSupabase`
- Used temporary fixed for the encountered problem that the `User` data class doesn't have a `uid` parameter - Fixed minor bug in accepting alert feature
…feat/alert/alertLists-ui-improvements # Conflicts: # app/src/main/java/com/android/periodpals/ui/alert/AlertLists.kt
…feat/alert/alertLists-ui-improvements
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.
Summary
LGTM
This PR significantly improves the UX of the AlertListsScreen()
by adding a top section for accepted alerts, removing the "Decline" button, and incorporating profile pictures for users. While it introduces some temporary solutions due to limitations in the User
data class, it ensures functional improvements and enhances the visual appeal.
Important
Code Quality
LGTM
- Readability: The code is generally well-structured, and the explanations in the description provide clear context.
- Consistency: The implementation aligns with the existing codebase patterns, except for the filtering workaround.
Functionality
LGTM
- Correctness: The new features (
acceptAlert()
andunAcceptAlert()
) appear to function as intended based on the description. However, filtering byname
poses risks for correctness when user names are not unique.
Performance
No significant performance concerns are noted.
Security
No security issues identified.
Testing
Good test coverage provided.
Documentation
Good documentation provided and the PR description is thorough and clearly explains the changes.
Suggestions
None
Steps Before PR Approval
LGTM
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.
Summary
Nice addition to the app! As you said, it's a bit sad that we couldn't make it persistent in time for the deadline, but this is still very useful as it sets the grounds for a later, more complete implementation. Good job!
Important
Code Quality
As always, your code is very readable, and consistent with the rest of the codebase!
Functionality
Works properly for the UI. As you said, it's not persistent but it works while it lasts, which was the aim of this PR.
Testing
All tests are up to date, and the added tests are correct, good job!
Documentation
Up to date as well, nice work!
Suggestions
See comments but it's all nitpicks.
Steps before PR approved
LGTM, thanks for the work (and I hope you had a good flight!)
app/src/main/java/com/android/periodpals/model/alert/AlertViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/android/periodpals/model/user/UserModelSupabase.kt
Show resolved
Hide resolved
app/src/main/java/com/android/periodpals/model/user/UserModelSupabase.kt
Show resolved
Hide resolved
app/src/main/java/com/android/periodpals/model/user/UserModelSupabase.kt
Show resolved
Hide resolved
app/src/main/java/com/android/periodpals/ui/alert/AlertLists.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/android/periodpals/ui/components/AlertComponents.kt
Outdated
Show resolved
Hide resolved
…feat/alert/alertLists-ui-improvements # Conflicts: # app/src/main/java/com/android/periodpals/ui/alert/AlertLists.kt # app/src/main/java/com/android/periodpals/ui/components/AlertComponents.kt # app/src/test/java/com/android/periodpals/ui/alert/AlertListsScreenTest.kt
Quality Gate passedIssues Measures |
AlertLists UI enhancement
Description
This PR introduces mainly new to the UI features for the
AlertListsScreen()
, to improve UX:PalsAlertItem
, leaving inly the "Accept" optionI didn't have the time to make this feature persistent, which means that if the user closes the app (the view model reset), the accepted alerts will not be remembered.
In order to implement that I would have had to modify the back end, probably add a column to the alerts table in supabase, to register the users (because they could be more than one) that accepted the alert. Then, fetch the information for each alert, comparing the uid of the user to see if they had accepted it.
This could have been done in a different PR, if we had the time.
Changes
AlertListsScreen()
, for profile picture manipulationdownloadFilePublic()
function, previously created in @Harrish92's PR Feat/alert/fetch profile picture of other users #339. Due to a misunderstanding he removed the function before merging, but it turns out we needed it, it just needed some modifications.LazyColumn
code inAlertListsScreen
, to add top sectionAlertProfilePicture
to display each user's correct profile picture (if existant)Important: we need to modify the
User
data class and model.for the fetching of the user's profile picture, it is done as follows:
loadUsers()
, which fillsuserViewModel.users
list with all theUser
s in the supabase repository.userViewModel.users
list, extract the one that corresponds to each alert card. Here is the problem: we should use a filter suchbut the
User
data class doesn't have a parameter uid, so for now I filter it by matching the name (which is really bad, but at least it works as a temporary solution).3) extract the image URL with
user.imageUrl
, etc.Files
Added
None
Modified
See files changed.
Removed
None
Dependencies Added
None
Testing
Added tests for the user model and view model function, same from @Harrish92's PR Feat/alert/fetch profile picture of other users #339
Added tests for
acceptAlert()
andunAcceptAlert()
inAlertViewModelTests
, which ensures the proper functionality of the states.Added tests for the functionality of accepting and un-accepting an alert in
AlertListsScreenTest
, i.e if the top section is effectively displayed, or notModified some of the pre-existing tests in
AlertListsScreenTest
: I couldn't get theassertIsDisplayed()
for the profile picture inmyAlertsListIsCorrect()
to pass. I have absolutely no idea why (specially because on the emulator we clearly see the profile picture displayed in "My Alerts" cards). I did my best to pinpoint the error but after 1 hour, I decided to move on... help on this is always appreciated.Screenshots (if applicable)