Skip to content

Show sending progress on ui - #1126

Merged
sosnovsky merged 4 commits into
masterfrom
feature/issue-1096-send-progress-status
Dec 3, 2021
Merged

Show sending progress on ui#1126
sosnovsky merged 4 commits into
masterfrom
feature/issue-1096-send-progress-status

Conversation

@Kharchevskyi

Copy link
Copy Markdown
Contributor

This PR update spinner during composing message

close #1096
https://user-images.githubusercontent.com/13693255/144126803-0837914c-6c6b-42c2-a232-867b6e6b1766.mp4


Tests (delete all except exactly one):

  • Does not need tests (refactor only, docs or internal changes)

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@tomholub tomholub left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Video looked good. See code comments

Comment thread FlowCrypt/Controllers/Compose/ComposeViewController.swift
@Kharchevskyi
Kharchevskyi marked this pull request as ready for review December 1, 2021 20:17
@tomholub
tomholub requested a review from sosnovsky December 1, 2021 20:57
@tomholub

tomholub commented Dec 1, 2021

Copy link
Copy Markdown
Collaborator

Roma - you are better suited to evaluate it.

@sosnovsky

Copy link
Copy Markdown
Collaborator

I see a several possible improvements:

  • app shows Composing label and then Composing XX%, probably second one should be replaced with Uploading XX%
  • also Composing can be not clear for users, as they've already composed message, it'll be better to say Encrypting instead
  • when message is sent users see Sent label with rotating spinner, which looks wrong - why it's rotating if message sending is completed. Previously we had tick icon when message is sent

Simulator Screen Shot - iPhone 12 - 2021-12-01 at 23 21 13

@Kharchevskyi

Copy link
Copy Markdown
Contributor Author

@sosnovsky fixed according to suggestions. Thanks
Also moved logic of showing hud based on the state directly to controller instead of update hud function

@sosnovsky

Copy link
Copy Markdown
Collaborator

Looks good now, the only issue I've noticed is that 'Uploading 100%stays for several seconds and thenSent` hud isn't visible, because it appears only when back transition starts, here is video:

Simulator.Screen.Recording.-.iPhone.12.-.2021-12-03.at.22.39.29.mp4

@Kharchevskyi

Copy link
Copy Markdown
Contributor Author

That is due to gmail api.
It sends progress 100 but actually message sends few milliseconds later.
I can try to improve ux for this. But this can be done either by postponing sending "sent" state from service or by delaying animation on conrtoller. Do you think it worth it? @tomholub

@sosnovsky

Copy link
Copy Markdown
Collaborator

I think previously Sent message was shown when progress reached 100%, and then back animation was performed when app receives successfully sent callback from Gmail.
So it was like a small fix/hack for not showing Uploading 100% for several seconds and showing Sent instead.

@Kharchevskyi

Copy link
Copy Markdown
Contributor Author

In this case "sent" is showing sometimes for few seconds

@sosnovsky

Copy link
Copy Markdown
Collaborator

But it seems more user-friendly than showing Uploading 100% for several seconds - for users it can look like something is wrong, everything is uploaded but nothing happens.

@Kharchevskyi

Copy link
Copy Markdown
Contributor Author

Would suggest either to keep this implementation or postpone hiding sent hud for some predefined duration 0.3 sec

@Kharchevskyi

Copy link
Copy Markdown
Contributor Author

Same with sent) a bit confusing. Message is sent. But nothing is happening for few seconds. Sometimes it takes up to 3 seconds. Which i would say looks like a bug

@sosnovsky

Copy link
Copy Markdown
Collaborator

Okay, let's leave it for now, as inbox screen already shows 'Encrypted message sent' popup after message is sent.

@sosnovsky
sosnovsky merged commit ec53b2d into master Dec 3, 2021
@sosnovsky
sosnovsky deleted the feature/issue-1096-send-progress-status branch December 3, 2021 21:15
@tomholub

tomholub commented Dec 3, 2021

Copy link
Copy Markdown
Collaborator

I agree with Roma here. I'll file another issue for him to improve it.

@tomholub

tomholub commented Dec 3, 2021

Copy link
Copy Markdown
Collaborator

Roma has good intuition when it comes to UX as well as code structure. When he raises a UX concern or code concern, we should all listen & pay attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

improve sending progress status to debug customer issue

3 participants