-
Notifications
You must be signed in to change notification settings - Fork 445
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
Implemented TaskCategory entity #355 #386
base: master
Are you sure you want to change the base?
Conversation
…hole stack, some basic unit tests carekit-apple#355
Thanks for the PR! We'll begin going over it and leaving feedback soon! |
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.
I've only just given this PR a first pass. I still have a lot to go over, but here are a couple items for you to work on in the mean time!
Thanks again for putting so much time and effort into this PR!
CareKit/CareKit/Lists/Controller/OCKTaskCategoriesListViewController.swift
Outdated
Show resolved
Hide resolved
...ed View Controllers/TaskCategory/ViewControllers/OCKDetailedTaskCategoryViewController.swift
Outdated
Show resolved
Hide resolved
CareKit/CareKit/Synchronized View Controllers/Utilities/OCKTaskCategoryUtility.swift
Outdated
Show resolved
Hide resolved
@@ -85,6 +85,6 @@ class TestSimpleContactViewSynchronizer: XCTestCase { | |||
viewSynchronizer.updateView(view, context: .init(viewModel: nil, oldViewModel: nil, animated: false)) | |||
XCTAssertNil(view.headerView.titleLabel.text) | |||
XCTAssertNil(view.headerView.detailLabel.text) | |||
XCTAssertEqual(view.headerView.iconImageView?.image, UIImage(systemName: "person.crop.circle")) | |||
XCTAssertEqual(view.headerView.iconImageView?.image, UIImage(systemName: "questionmark.circle.fill")) |
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.
I think we may want to keep the default image as "person.crop.circle" for the contact views.
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.
Yes, and we are using that. I introduced the questionmark icon there so that we can use it as a universal placeholder.
If you look at OCKHeaderView+Updatable, you'll see the updated logic. We are using OCKHeaderView for various purposes - Events, Tasks, Contacts, and TaskCategories. If the clear out and not configured state was using a person placeholder, it would look strange in the TaskCategory list.
The Contact list will not be impacted by this change. The difference is in the configuration. If you configure the OCKHeaderView with a OCKAnyContact that doesn't have any asset - the image will be rendered as a person placeholder. In case of the OCKAnyTaskCategory, a question mark image will be used.
Let me know if this is desirable logic or what other solution would you suggest.
CareKitUI/CareKitUI/Components/TaskCategory/Button/OCKTaskCategoryButton.swift
Outdated
Show resolved
Hide resolved
CareKitUI/CareKitUI/Common/Views/OCKTaskCategoryHeaderView.swift
Outdated
Show resolved
Hide resolved
CareKitUI/CareKitUI/Supporting Files/Localization/en.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
@erik-apple for some reason the CI is giving me a failure in some of the unit tests, although they pass when I run them locally. Are you able to reproduce?
|
Yes, that’s a known issue. We’ve noticed that it happens at a certain time of day, and only on Travis’s CI servers. We are unable to reproduce it locally as well. If you wait a couple hours and rerun the tests they will pass. Don’t worry about it for now though, we’ll ensure they pass before we merge. |
@erik-apple Hi Erik, it's been awhile, would you mind giving me some guidance so that I can make all required alterations to get this PR merged? Thanks in advance! |
Addresses issue #355.
A new TaskCategory entity was introduced. This feature helps to organize OCKTasks in categories.
This way the user can categorize the tasks by a medical condition, by the type of action, or in any other way they want.
Database modifications
New entities
New relationships
Considerations
Shouldn't we add an "Uncategorized" TaskCategory that will always be there and all the Tasks that are not associated with any TaskCategory will be displayed when this "Uncategorized" TaskCategory is selected?
...
Currently, the taskCategory property is optional. To implement the "Uncategorized" TaskCategory would mean adding a special logic for this use-case. A more generic solution is to solve this issue upon task creation. If you create a new task, you'll choose a category. In no category is selected, there will be the default "Uncategorized" TaskCategory. But this logic is something to be decided by the end-user (the programmer of a particular app). Some apps might not want to categorize the tasks, so the default category would render no benefit.
Nesting of the categories
...
It's a bit tricky because Swift structs cannot be nested directly (to prevent infinite loop upon initialization). The alternative is to use IDs. Is this something that is desirable? What is the best way to achieve nesting?
Gravirawson's suggestion [Proposal] Category Card, OCKTaskCategory, organization of OCKTasks #355 (comment): create an OCKCategory instead of OCKTaskCategory for CarePlans and other use-cases.
...
What would be the use-case for the care plan category? Could be the same category reused in multiple different entities?
Should the TaskCategory to Task relationship be many-to-many instead?
...
This would provide more flexibility. Right now, I only have one task category per task for simplicity's sake. The many-to-many would enable to filter tasks by different sets of categories. This is especially useful for the implementation of the "All tasks" category (that we can include in the list of categories).
What is the best way to integrate this new functionality in terms of UI?
...
Really an open question for debate. I created a very basic category selection view controller that in turn presents the daily list of tasks filtered by a task category. Should the filtering happen on the main screen? Or would a completely different UI be more appropriate? The original idea was to create the same drill-down experience as in the Apple Health app.
Should we keep the notes array in the OCKTaskCategory?