remove default data from RelationView #154
Conversation
If the viewmodel is in Activity mode then only ActivityIndicator will be shown. If there is any relation then the Relation data would be shown. Otherwise, a default text will be shown to indicate an empty relation.
There was a problem hiding this comment.
Hi @Zoha131! Thanks a lot for submitting a PR for this. The change looks great to me and thanks for fixing this bug for us.
I have added a couple of generic comments and request some minor changes. Let me know what you think?
| @@ -45,6 +45,7 @@ struct LocalizableStringConstants { | |||
| static let profile = LocalizedStringKey("Profile") | |||
| static let editProfile = LocalizedStringKey("Edit Profile") | |||
| static let addTask = LocalizedStringKey("Add Task") | |||
| static let noRelationText = LocalizedStringKey("No active relation") | |||
There was a problem hiding this comment.
nit: Could change to "No active relations"
There was a problem hiding this comment.
I will rename the variable.
| return relationViewModel.currentRelation.id != nil && | ||
| relationViewModel.currentRelation.id != 0 |
There was a problem hiding this comment.
Question: Why are you checking for both nil and 0?
Wouldn't nil signify that the backend response was not found in which case showing the user the "No active relation" state is wrong and we should show an error state instead?
There was a problem hiding this comment.
currentRelation inside RelationModel has default id which is 0. And if there is no relation then the response I get is nil. This is why I am checking both.
There was a problem hiding this comment.
Alright..thanks for clarifying!
| TasksSection(tasks: relationViewModel.doneTasks, navToTaskComments: true) | ||
| } | ||
| } else { | ||
| Text(LocalizableStringConstants.noRelationText) |
There was a problem hiding this comment.
Possibly overkill but something to consider:
This kind of view where the window is Empty because there is no content is a pretty common state for an app and we should have a generic UtilityView for it. Can you add a generic UtilityView like the ActivityIndicator and pass a message as a variable? It would make it reusable and incase we want to later add some icons we could do that too.
There was a problem hiding this comment.
I think this is a good idea. I have seen some other screens where an empty view can be re-used.
| TasksSection(tasks: relationViewModel.doneTasks, navToTaskComments: true) | ||
| } | ||
| } else { | ||
| Text(LocalizableStringConstants.noRelationText) |
There was a problem hiding this comment.
Add a background color here in this view.
There was a problem hiding this comment.
Do you want a background color for the empty view? If yes then should I add a background color forActivityIndicator as well?
There was a problem hiding this comment.
Yes sure, lets add background color to both.
There was a problem hiding this comment.
Thanks for the confirmation. I will update my PR in a few days.
…into noRelation Merging
Description
If the viewmodel is in Activity mode then only ActivityIndicator will be shown. If there is any relation then the Relation data would be shown. Otherwise, a default text will be shown to indicate an empty relation.
Fixes #144
Type of Change:
How Has This Been Tested?
I have manually tested the screens.
Checklist: