Skip to content

Added comments to setup flow controllers - #368

Merged
tomholub merged 38 commits into
masterfrom
feature/issue-460-add-comments
Jun 24, 2021
Merged

Added comments to setup flow controllers#368
tomholub merged 38 commits into
masterfrom
feature/issue-460-add-comments

Conversation

@Kharchevskyi

@KharchevskyiKharchevskyi commented Jun 22, 2021•
edited by tomholub
Loading

Copy link
Copy Markdown
Contributor

This PR adds comments blocks to Setup Flow ViewControllers

issue #360

Tests:

  • 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 previously approved these changes Jun 22, 2021
View reviewed changes

@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 - thank you.

This does not close the issue yet - please add similar comments to all view controllers. This will make reviews a lot easier. Thanks!

@tomholub
tomholub enabled auto-merge (squash) June 22, 2021 20:05
@Kharchevskyi
Kharchevskyi marked this pull request as draft June 22, 2021 20:08
auto-merge was automatically disabled June 22, 2021 20:08

Pull request was converted to draft

tomholuband others added 20 commits June 23, 2021 05:23
@tomholub
Merge branch 'master' into feature/issue-460-add-comments
427a826
@tomholub
Merge branch 'master' into feature/issue-460-add-comments
d08dc83
@Kharchevskyi
Update MyMenuViewController
3f73d47
@Kharchevskyi
Update SideMenuNavigationController
2c58890
@Kharchevskyi
Update MainNavigationController
a4c258d
@Kharchevskyi
Update EmailProviderViewController
99fcb12
@Kharchevskyi
Update SignInViewController
44272e4
@Kharchevskyi
Update WebViewController
5a23c5e
@Kharchevskyi
Update BootstrapViewController
719972e
@Kharchevskyi
Update InboxViewController
dc47538
@Kharchevskyi
Update InboxViewControllerContainer
b9478ae
@Kharchevskyi
Update ComposeViewController
40ace0c
@Kharchevskyi
Update MessageViewController
18fa470
@Kharchevskyi
Update SearchViewController
6fd4784
@Kharchevskyi
Update ContactsListViewController
7625008
@Kharchevskyi
Update ContactDetailViewController
c1426c0
@Kharchevskyi
Update KeySettingsViewController
6b42a9a
@Kharchevskyi
Update KeyDetailViewController
a69ea04
@Kharchevskyi
Update PublicKeyDetailViewController
9b29660
@Kharchevskyi
Update KeyDetailInfoViewController
2d8e242
@Kharchevskyi
Kharchevskyi marked this pull request as ready for review June 24, 2021 20:03
@KharchevskyiKharchevskyi linked an issue Jun 24, 2021 that may be closed by this pull request
add block comment about user context for each VC #360
Closed
tomholub
tomholub reviewed Jun 24, 2021
View reviewed changes

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

Looks good!

Comment threadFlowCrypt/Controllers/Inbox/Container/InboxViewControllerContainer.swift Outdated
Comment threadFlowCrypt/Controllers/SignIn Other/EmailProviderViewController.swift
* - User redirects here from SignInViewController
* - After successful connection user will be redirected to *main flow*
*/
final class EmailProviderViewController: TableNodeViewController {

@tomholubtomholubJun 24, 2021

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.

We should call this SetupImapViewController ?

@KharchevskyiKharchevskyiJun 24, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Maybe better to call it SetupEmailProviderViewController? or if you think SetupImapViewController is better. naming - I'm ok with that as well

@tomholubtomholubJun 24, 2021

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.

This one is specifically for imap and smtp. I think email provider is too generic here. SetupImapViewController is preferred

@KharchevskyiKharchevskyiJun 24, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

will change then in scope of #371

@tomholub
tomholub enabled auto-merge (squash) June 24, 2021 20:17
@tomholubtomholub mentioned this pull request Jun 24, 2021
Closed
tomholub
tomholub previously approved these changes Jun 24, 2021
View reviewed changes
@tomholub
tomholub merged commit 18a39f3 into masterJun 24, 2021
@tomholub
tomholub deleted the feature/issue-460-add-comments branch June 24, 2021 20:34
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomholubtomholubtomholub approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

add block comment about user context for each VC

2 participants

@Kharchevskyi@tomholub