Supplies Screen #182
Supplies Screen #182
Conversation
|
According to the design system that was choosen, different types of text field should not be next to each other: https://m2.material.io/components/text-fields#usage
"Unassigned myself" is a bit weird; here are some alternatives:
|
| stringResource(id = R.string.chimpagne_cancel), | ||
| onClick = onDismissRequest, | ||
| modifier = Modifier.testTag("guest_supply_cancel")), | ||
| if (supply.assignedTo.containsKey(loggedUserUID)) |
There was a problem hiding this comment.
if-else statements without brackets should not be used here, according to Kotlin's style guide :
| stringResource(id = R.string.supplies_supply_assigned_to), | ||
| modifier = Modifier.testTag("staff_list")) | ||
| } | ||
| accounts.entries.forEach { (userUID, userAccount) -> |
There was a problem hiding this comment.
A filter would be more approriate
There was a problem hiding this comment.
Done, 🇧🇸 would be proud
| if (!tempSupply.assignedTo.keys.containsAll(accounts.keys)) { | ||
| item { Text(stringResource(id = R.string.supplies_not_assigned_to)) } | ||
| accounts.entries.forEach { (userUID, userAccount) -> | ||
| if (tempSupply.assignedTo[userUID] != true) { |
There was a problem hiding this comment.
!= true should be changed to !tempSupply.assignedTo[userUID]
There was a problem hiding this comment.
tempSupply.assignedTo: Map<ChimpagneAccountUID, Boolean?> thus !tempSupply.assignedTo[userUID] doesn't work as tempSupply.assignedTo[userUID] can be null
| title = { Text(stringResource(id = R.string.supplies_screen_title)) }, | ||
| modifier = Modifier.shadow(4.dp), | ||
| navigationIcon = { | ||
| IconButton(onClick = { navObject.goBack() }) { |
There was a problem hiding this comment.
The reusable component GoBackButton should be used here:
There was a problem hiding this comment.
amazing component, thx
| text = "${supply.quantity} ${supply.unit}", | ||
| modifier = Modifier.testTag("supply_quantity_and_unit")) | ||
| Text( | ||
| text = "${supply.assignedTo.keys.size} assigned", |
There was a problem hiding this comment.
A resource string should be used here.
| headlineContent = { | ||
| Text( | ||
| text = | ||
| "${account?.firstName} ${account?.lastName}" + |
| @@ -16,6 +16,7 @@ object Route { | |||
| const val MY_EVENTS_SCREEN = "myEvents" | |||
| const val VIEW_DETAIL_EVENT_SCREEN = "viewDetailEventScreen" | |||
| const val MANAGE_STAFF_SCREEN = "manageStaffScreen" | |||
| const val SUPPLIES_SCREEN = "SuppliesScreen" | |||
There was a problem hiding this comment.
the previous values use a lowercase character as the first character
There was a problem hiding this comment.
It seems a bit anarchic
A future PR will solve this issue.
# Conflicts: # app/src/main/res/values-fr/strings.xml # app/src/main/res/values/strings.xml
There was a problem hiding this comment.
As stated by other reviewer this is good PR overall, the UI is great looking and the tests are very robust.
But I have to point out a huge oversight.
When a guest or staff member leaves the event, their assigned supplies are still stored in the database.
This shouldn't happen in my opinion, once a person from the event decides to leave, they should automatically get removed from all supplies they were assigned to.
Thus this change should be addressed before merging.
This is a UI only PR... However I fixed this |
|
Athanaze
dismissed
juaniac’s stale review
we need to move forward, your comments are very interesting but not in the scope of this PR
This PR adds the supplies screen to the detailed event page. From there, if you're a staff, you can add / edit / delte / assign supplies. As a guest, you can only assign / unassign yourself from a supply.
A new route (SUPPLIES_SCREEN) has been added.
Here are some screens:
Staff perspective
Guest perspective
