Skip to content
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

Updated reply and edit comment messaging experience #11817

Merged
merged 7 commits into from May 31, 2019

Conversation

@yaelirub
Copy link
Contributor

commented May 30, 2019

This PR only updates the experience of messaging the failure in uploading an edit to a comment or a reply to a comment and does not deal with the reported failure to upload the comment completely.

Instead of showing an alert with confusing options to try again (which didn't do anything) or to cancel (which dismissed the alert)
We are showing a snack bar to fit the behavior of this type of failure in other parts of the app. w

Hopefully the failed upload of the comment/reply can be automatically triggered in the future with the upcoming addition of the upload manager

Fixes #11307

To test:

  1. Turn on Network Link Conditioner with 100% loss
  2. Go to comments
  3. reply to a comment
  • See the snack bar with a message saying "There has been an unexpected error while sending your reply"

Before:
reply_old

After:
reply_new

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@yaelirub yaelirub added this to the 12.6 milestone May 30, 2019

@yaelirub yaelirub requested a review from shiki May 30, 2019

@yaelirub yaelirub added this to Backlog in Offline Support: Posting [iOS] via automation May 30, 2019

Offline Support: Posting [iOS] automation moved this from Backlog to In Review May 30, 2019

@shiki
Copy link
Member

left a comment

Thank you for working on this, @yaelirub. I'm requesting changes here because of the UIAlertController extension, marked with ⚠️.

Possible Improvements

There are opportunities for improvements in the UI.

The message disappears as soon as the I hit Reply

Very long response time

On a very bad connection, it takes a while to receive an error or success message. On this GIF, it takes roughly 1 minute before I received a response.

These are scary behaviors. As a user, I'm not sure if something is happening in the background. If there was an error, I could not even get my message back. It is probably not trivial to fix these. Should we make this part of the Offline Support project? Or is it out of scope?

For now, what do you think about these?

  1. Show a Notice that we are sending the comment. We can experiment with using NoticeStyle.isDismissable but that might make things more difficult for now.
  2. Replace the "Reply Sent!" HUD with a Notice. That seems like a low hanging fruit. 😉

@osullivanchris, perhaps you'd like to chime in on this as well.

@@ -53,3 +53,13 @@ extension UIAlertController {
copyAlertController?.presentFromRootViewController()
}
}

@objc extension UIAlertController {

This comment has been minimized.

Copy link
@shiki

shiki May 30, 2019

Member

⚠️ I don't agree with these methods being part of UIAlertController. They're essentially a different way of showing messages and have no relation to UIAlertController whatsoever. I suggest we reuse the ReachabilityUtils methods instead.

    [ReachabilityUtils showNoInternetConnectionNoticeWithMessage:message];

    [ReachabilityUtils dismissNoInternetConnectionNotice];

It should be fine for this use case.

This comment has been minimized.

Copy link
@yaelirub

yaelirub May 30, 2019

Author Contributor

This feels like overloading ReachabilityUtils since this notices can be displayed on any error. Granted most of the time this error would be due to a bad/ no connection so I'm willing to use that but we should figure out another way to use the notices in objc. Maybe outside the scope of this

This comment has been minimized.

Copy link
@yaelirub

yaelirub May 30, 2019

Author Contributor

This feels like overloading ReachabilityUtils since this notices can be displayed on any error. Granted most of the time this error would be due to a bad/ no connection so I'm willing to use that but we should figure out another way to use the notices in objc. Maybe outside the scope of this

This comment has been minimized.

Copy link
@shiki

shiki May 30, 2019

Member

Granted most of the time this error would be due to a bad/ no connection so I'm willing to use that but we should figure out another way to use the notices in objc.

Yes, that's why I was suggesting to use Reachability. Regarding Obj-C, I wish we could convert the call sites to Swift instead. 😄 But it's definitely not trivial.

This comment has been minimized.

Copy link
@yaelirub

yaelirub May 31, 2019

Author Contributor

Yes, that's why I was suggesting to use Reachability

@shiki ,not sure I understand you here. You're suggesting to use ReachabilityUtils to show a notice with showNoInternetConnectionNoticeWithMessage regardless if it's because of no internet connection.
For example, I'd like to display a notice when the reply was successfully sent, instead of a HUD. Should I use showNoInternetConnectionNoticeWithMessage in that case too?

This comment has been minimized.

Copy link
@shiki

shiki May 31, 2019

Member

Should I use showNoInternetConnectionNoticeWithMessage in that case too?

Good point! I have forgotten that we cannot use Notices in Obj-C.

For the error messages, let's still use ReachabilityUtils, not the UIAlertController extensions. As for the HUD, we can figure that out until we have a better solution for using Notices in Obj-C.

This comment has been minimized.

Copy link
@yaelirub

yaelirub May 31, 2019

Author Contributor

@shiki, I actually think I’m gonna to use Nate’s suggestion: #11441 (comment)
And add those methods in UIViewController+Notice is you have no objection

This comment has been minimized.

Copy link
@shiki

shiki May 31, 2019

Member

No objection! 🙂

@yaelirub

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

So weird. I wrote a comment while I was at the airport and I can't see it here. I guess Github has there own issues on low connectivity :P

Show a Notice that we are sending the comment. We can experiment with using NoticeStyle.isDismissable but that might make things more difficult for now.

I'm not sure about this. The way we use notices in the app is to notify when an action was successful or not and not that we are preforming an action so it doesn't feel aligned.
I agree the behavior is not great but I'd like to leave this out of the scope for this PR.
Maybe I will try and not delete the input text field content till we're coming back from the upload call.

Replace the "Reply Sent!" HUD with a Notice. That seems like a low hanging fruit. 😉
Great suggestion. I'll update

@yaelirub yaelirub force-pushed the issue/11307-unable_to_send_reply_error_message branch from a004f79 to 2d6a129 May 31, 2019

@shiki

This comment has been minimized.

Copy link
Member

commented May 31, 2019

I'm not sure about this. The way we use notices in the app is to notify when an action was successful or not and not that we are preforming an action so it doesn't feel aligned.

Hmm, yeah. That makes sense.

I agree the behavior is not great but I'd like to leave this out of the scope for this PR.

Sounds good!

@yaelirub

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Commenting here too since the comments are on an outdated.
@shiki, I actually think I’m gonna to use Nate’s suggestion: #11441 (comment)
And add those methods in UIViewController+Notice if you have no objection


// Note: This viewController might not be visible anymore
alertController.presentFromRootViewController()
let message = NSLocalizedString("There has been an unexpected error while sending your reply", comment: "Reply Failure Message");

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 31, 2019

Trailing Semicolon Violation: Lines should not have trailing semicolons. (trailing_semicolon)

let notice = Notice(title: title, message: message)
ActionDispatcher.dispatch(NoticeAction.post(notice))
}

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 31, 2019

Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)

@@ -33,3 +33,15 @@ extension UIViewController {
ActionDispatcher.dispatch(NoticeAction.post(notice))
}
}

@objc extension UIViewController {

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 31, 2019

Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)


// Note: This viewController might not be visible anymore
alertController.presentFromRootViewController()
let message = NSLocalizedString("There has been an unexpected error while sending your reply", comment:"Reply Failure Message");

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 31, 2019

Colon Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. (colon)
Trailing Semicolon Violation: Lines should not have trailing semicolons. (trailing_semicolon)

@yaelirub yaelirub force-pushed the issue/11307-unable_to_send_reply_error_message branch from 52f563a to 1293748 May 31, 2019

yaelirub added some commits May 30, 2019

Updated reply and edit comment failure experience
Instead of showing an alert with confusing options to try again (which didn't do anything) or to cancel (which dismissed the alert)
We are showing a snack bar to fit the behavior of this type of failure in other parts of the app. w

Hopefully the failed upload of the comment/reply can be automatically triggered in the future with the upcoming addition of the upload manager
Displaying notice on success and failure to reply
- Added a UIViewController extension to show and dismiss notices.
- Displaying successful and failure notices in comments, notification and reader view controllers

@yaelirub yaelirub force-pushed the issue/11307-unable_to_send_reply_error_message branch from 1293748 to 61133e0 May 31, 2019

@yaelirub yaelirub force-pushed the issue/11307-unable_to_send_reply_error_message branch from 61133e0 to 91355d9 May 31, 2019

@yaelirub

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Hey @shiki , I updated the PR to show notices on success or failure of a reply.
I think the commenting experience should be the same everywhere, so I update NotificationViewController and ReadCommentsViewController to have the same experience.
Ready for another look 🙏

@yaelirub yaelirub changed the title Updated reply and edit comment failure experience Updated reply and edit comment messaging experience May 31, 2019

@shiki

shiki approved these changes May 31, 2019

Copy link
Member

left a comment

Thank you, @yaelirub. :shipit:

@@ -33,3 +33,14 @@ extension UIViewController {
ActionDispatcher.dispatch(NoticeAction.post(notice))
}
}

@objc extension UIViewController {
@objc func displayNotice(title: String, message: String? = nil) {

This comment has been minimized.

Copy link
@shiki

shiki May 31, 2019

Member

A possible improvement in the future is to include a tag so that dismissNotice() will only dismiss Notices dispatched by the current ViewController.

@yaelirub yaelirub added General and removed General labels May 31, 2019

@yaelirub yaelirub merged commit 1b63b54 into develop May 31, 2019

3 checks passed

Hound No violations found. Woof!
Peril All green. Well done.
Details
ci/circleci: build_and_test Your tests passed on CircleCI!
Details

Offline Support: Posting [iOS] automation moved this from In Review to Done (PRs) May 31, 2019

@peril-wordpress-mobile

This comment has been minimized.

Copy link

commented May 31, 2019

Fails
🚫

Could not find Dangerfile at org/common.ts on wordpress-mobile/peril-settings on branch master

Full state of PR run:

{
"branch": null,
"dangerfileBranchForPR": "issue/11307-unable_to_send_reply_error_message",
"neededDangerfileIsLocalRepo": false,
"repoForDangerfileRun": "wordpress-mobile/peril-settings",
"run": {
  "action": "closed",
  "dslType": 0,
  "event": "pull_request",
  "branch": "master",
  "dangerfilePath": "org/common.ts",
  "repoSlug": "wordpress-mobile/peril-settings",
  "referenceString": "wordpress-mobile/peril-settings@org/common.ts",
  "feedback": 0
},
"settings": {
  "commentableID": 11817,
  "eventID": "Unknown",
  "hasRelatedCommentable": true,
  "installationID": 678301,
  "installationSettings": {
    "env_vars": [],
    "ignored_repos": [
      "wordpress-mobile/peril-settings"
    ],
    "modules": []
  },
  "isRepoEvent": true,
  "isTriggeredByUser": true,
  "repoName": "wordpress-mobile/WordPress-iOS",
  "repoSpecificRules": {
    "pull_request, pull_request.labeled, pull_request.unlabeled": "wordpress-mobile/peril-settings@org/ios.ts"
  },
  "triggeredByUsername": "yaelirub"
}
}

Generated by 🚫 dangerJS

@diegoreymendez

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

@yaelirub - Can this branch be deleted?

@yaelirub yaelirub deleted the issue/11307-unable_to_send_reply_error_message branch Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.