-
Notifications
You must be signed in to change notification settings - Fork 443
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
allow activities to be discontinued. refs #71 #80
Conversation
Hi: |
@scdi |
@@ -474,6 +474,20 @@ - (void)tableView:(UITableView *)tableView didSelectRowAtIndexPath:(NSIndexPath | |||
} | |||
|
|||
|
|||
- (void)tableView:(UITableView *)tableView commitEditingStyle:(UITableViewCellEditingStyle)editingStyle forRowAtIndexPath:(NSIndexPath *)indexPath { | |||
OCKCarePlanActivity *selectedActivity = _events[indexPath.row].firstObject.activity; | |||
OCKCarePlanEvent *event = _events[indexPath.row].firstObject; |
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.
Refactor to:
OCKCarePlanEvent *event = _events[indexPath.row].firstObject;
OCKCarePlanActivity *activity = event.activity;
@dukemike Minor nits. Overall looks great! 👍 The Discontinue Button shows up even after the user has tapped to discontinue the activity. @YuanZhu-apple Could you also take a look? :) |
…vent.date = activity.endDate
@umerkhan-apple, @YuanZhu-apple Please review. Thanks! |
|
||
@param viewController The view controller providing the callback. | ||
@param interventionActivity The intervention activity that the user selected. | ||
*/ |
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.
Update documentation to state that the discontinue will happen after "today's" date. And this is only applicable to the current day. If the user goes back to a previous day, they cannot mark it to be discontinued.
@dukemike Looks great. Thanks for the updating the test app as well. One thing that is still not fixed is that the user can go back to a previous day, and swipe left on an activity to discontinue it. Apart from that the PR looks great. Once that fix is in, this one is good to be merged. |
Being able to discontinue an activity on a prior day is something that I'd like to allow. Could preventing discontinuation in the past be enforced by the developer vs the framework? I can imagine the case where the user stopped performing an activity yesterday but didn't open the app to discontinue it then. |
@dukemike Ah that use-case definitely make sense. My concern is that there is no way to allow the developer to discontinue in the past or not. The delegate callback provides the Activity. One bad path is: Any creative ideas on how we can go around this? |
Also, one other thing I thought of. Could we just have the button say |
@umerkhan-apple The idea of checking for future completed events for an activity to determine if the discontinue should be shown sounds both problematic and non-performant. How about if a user tries to discontinue and there are future completed events then: the activity's completion date could be set to after the last completed event, or the user could be asked if they want to delete the future ones? I lean towards asking the user. re: the discontinuation text / date, my preference is that discontinuation is something that end the activity and does not remove it from view because it feels less like Delete to me, but I'm not tied to it and don't have any major concerns if we set the endDate to yesterday. "Delete" and "Discontinue after today" feel clear to me, while "Discontinue" feels less so, but I agree that it is really wordy. |
I played with this a little bit, and I think it's a great addition (next up is adding activities!) However, I do think that in addition to having it available on swipe, it should be available on the default detail view controller of interventions (assuming the delegate method is implemented). As for the naming, "Discontinue After Today" is quite long (more than half the width on iPhone 6s). I was thinking maybe "End Today" or "Discontinue" like what @umerkhan-apple said. As for the error case, there are one of two ways we could go about this:
|
@dukemike I think for the wording, it would make sense to just say This really gets tricky if it is something that is only once a month. So on the first of every month, they go for physical therapy. Then when they complete that task, they know they will no longer be repeating it the following month, but most likely users won't remember to mark it as discontinue. Cause they will have already logged it as something that they have done. Come next month, they see the activity again. Now if they mark it as discontinued, it won't show up the following month, but it still will for the current month. At this point, they will have to go back to the previous month's activity, and mark that as discontinued. This approach seems very counter-intuivite. I think we should just have the button say For the case where they are going back in time to discontinue something, we can show them an alert confirming that they will lose all the data from that day onwards. If they confirm, then we set the end date and make sure that no data is stored past that end date for the activity. |
That seems very reasonable to me. That is a good approach. Sent from my iPhone
|
Sounds good. I'll set the endDate to the previous date and make the other changes. |
Closing this PR as it is no longer relevant due to CareKit 2.0 beta update, however we are discussing internally to provide support for discontinuing tasks... |
No description provided.