Skip to content

Feature/issue 695 upload progress - #773

Merged
tomholub merged 9 commits into
masterfrom
feature/issue-695-upload-progress
Oct 25, 2021
Merged

Feature/issue 695 upload progress#773
tomholub merged 9 commits into
masterfrom
feature/issue-695-upload-progress

Conversation

@sosnovsky

Copy link
Copy Markdown
Collaborator

This PR adds progress bar for emails sending process.

close#695


Tests(delete all except exactly one):

  • Difficult to test (explain why) - it's UI change

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

@tomholubtomholub 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.

Code looks good 👍 will test


extension GmailService: MessageGateway {
func sendMail(input: MessageGatewayInput) async throws {
func sendMail(input: MessageGatewayInput, progressHandler: ((Float) -> Void)?) async throws {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe it will be easy to have one shared instance of GmailService and set progressHandler directly without setting it via sendMail?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yes, it can be a possible solution, but as for me - having progressHandler along with sendMail makes it easier to follow mail sending flow.

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.

Agree with Roma here, passing it together in sendMail makes it more obvious how it's used and why.

@tomholubtomholub 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.

Excellent 👍

@tomholub
tomholub merged commit fc1833f into masterOct 25, 2021
@tomholub
tomholub deleted the feature/issue-695-upload-progress branch October 25, 2021 20:54
@tomholubtomholub mentioned this pull request Oct 25, 2021
5 tasks
Sign up for freeto 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 visual progress when sending message with attachments

3 participants

@sosnovsky@ivan-ushakov@tomholub