Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

remove default data from RelationView #154

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

remove default data from RelationView #154

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8025510
remove default data from RelationView to fix #144
Zoha131 Oct 28, 2020
936c410
Merge branch 'develop' of https://github.com/anitab-org/mentorship-io…
Zoha131 Jan 25, 2021
a9092c8
Add EmptyIndicator Utility view
Zoha131 Jan 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mentorship ios/Constants/LocalizableStringConstants.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could change to "No active relations"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will rename the variable.

static let markComplete = LocalizedStringKey("Mark as complete")
static let relationRequest = LocalizedStringKey("Relation Request")
static let notAvailable = LocalizedStringKey("Not available")
Expand Down
87 changes: 47 additions & 40 deletions mentorship ios/Views/Relation/Relation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ struct Relation: View {
var endDate: Date {
return Date(timeIntervalSince1970: relationViewModel.currentRelation.endDate ?? 0)
}

var hasRelation: Bool {
return relationViewModel.currentRelation.id != nil &&
relationViewModel.currentRelation.id != 0
Comment on lines +20 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright..thanks for clarifying!

}

// use service to fetch relation and tasks
func fetchRelationAndTasks() {
Expand Down Expand Up @@ -69,49 +74,51 @@ struct Relation: View {
var body: some View {
NavigationView {
ZStack {
Form {
//Top detail view
VStack(alignment: .leading, spacing: DesignConstants.Form.Spacing.minimalSpacing) {
//mentor/mentee name and end date
HStack {
Text(relationViewModel.personName).font(.title).fontWeight(.heavy)
Spacer()
Text(relationViewModel.personType).font(.title)
}
.foregroundColor(DesignConstants.Colors.subtitleText)

Text("Ends On: \(DesignConstants.DateFormat.mediumDate.string(from: endDate))")
.font(.callout)

//divider, adds a line below name and date
Divider()
.background(DesignConstants.Colors.defaultIndigoColor)
}
.listRowBackground(DesignConstants.Colors.formBackgroundColor)

//Tasks To Do List section
TasksSection(tasks: relationViewModel.toDoTasks, isToDoSection: true, navToTaskComments: true) { task in
//set tapped task
RelationViewModel.taskTapped = task
//show alert for marking as complete confirmation
self.showAlert.toggle()
}
.alert(isPresented: $showAlert) {
Alert(
title: Text(LocalizableStringConstants.markComplete),
primaryButton: .cancel(),
secondaryButton: .default(Text(LocalizableStringConstants.confirm)) {
self.markAsComplete()
})
}

//Tasks Done List section
TasksSection(tasks: relationViewModel.doneTasks, navToTaskComments: true)
}

//show activity spinner if in activity
if relationViewModel.inActivity {
ActivityIndicator(isAnimating: $relationViewModel.inActivity, style: .medium)
} else if hasRelation {
Form {
//Top detail view
VStack(alignment: .leading, spacing: DesignConstants.Form.Spacing.minimalSpacing) {
//mentor/mentee name and end date
HStack {
Text(relationViewModel.personName).font(.title).fontWeight(.heavy)
Spacer()
Text(relationViewModel.personType).font(.title)
}
.foregroundColor(DesignConstants.Colors.subtitleText)

Text("Ends On: \(DesignConstants.DateFormat.mediumDate.string(from: endDate))")
.font(.callout)

//divider, adds a line below name and date
Divider()
.background(DesignConstants.Colors.defaultIndigoColor)
}
.listRowBackground(DesignConstants.Colors.formBackgroundColor)

//Tasks To Do List section
TasksSection(tasks: relationViewModel.toDoTasks, isToDoSection: true, navToTaskComments: true) { task in
//set tapped task
RelationViewModel.taskTapped = task
//show alert for marking as complete confirmation
self.showAlert.toggle()
}
.alert(isPresented: $showAlert) {
Alert(
title: Text(LocalizableStringConstants.markComplete),
primaryButton: .cancel(),
secondaryButton: .default(Text(LocalizableStringConstants.confirm)) {
self.markAsComplete()
})
}

//Tasks Done List section
TasksSection(tasks: relationViewModel.doneTasks, navToTaskComments: true)
}
} else {
Text(LocalizableStringConstants.noRelationText)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good idea. I have seen some other screens where an empty view can be re-used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a background color here in this view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want a background color for the empty view? If yes then should I add a background color forActivityIndicator as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sure, lets add background color to both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the confirmation. I will update my PR in a few days.

}
}
.environment(\.horizontalSizeClass, .regular)
Expand Down