This repository was archived by the owner on Feb 5, 2025. It is now read-only.
V. 1.10.7-beta.1 - Magic Email Clients #178
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 4 commits
January 22, 2020 15:26
- add AppSelector type: wraps a custom UIAlertController that contains a list of apps that can be called - add LinkMailPresenter: uses AppSelector to present a list of available email clients. The supported clients are read from EmailClients.plist - update NUXLinkMailViewController, to use LinkMailPresenter
…update AppSelector to sort the action list alphabetically and use WordPressAuthenticator.bundle to search EmailClients.plist
- add URLHandler protocol - add conformance to URLHandler for URLSession to inject the dependency in AppSelector - add AppSelectorTests.swift
2 tasks
frosty
reviewed
Jan 24, 2020
Contributor
frosty
left a comment
frosty
left a comment
There was a problem hiding this comment.
This is looking really good! I just left some comments in the code, but nothing too major :)
WordPressAuthenticator/Email Client Picker/LinkMailPresenter.swift
Outdated
Show resolved
Hide resolved
* Update AppSelectorTitles: add context to localizable strings * Update LinkMailPresenter: update message to check email, using the user's email address
Contributor
Author
|
@frosty , @ScoutHarris , catching up on this after my vacation break! Please let me know if there are any other changes needed before approving it. Thanks! |
frosty
reviewed
Feb 4, 2020
Contributor
frosty
left a comment
frosty
left a comment
There was a problem hiding this comment.
Looking good, I just have one comment about localization!
| let message = NSLocalizedString("Please open your email app and look for an email from WordPress.com.", comment: "Message to ask the user to check their email and look for a WordPress.com email.") | ||
| let title = NSLocalizedString("Check your email!", | ||
| comment: "Alert title for check your email during logIn/signUp.") | ||
| let message = NSLocalizedString("We just emailed a link to ", |
Contributor
There was a problem hiding this comment.
I'd suggest using a localized string with format here, so that these strings can be combined into one – different languages may need to put the email address into a different order in the string, so it's easiest for translation if it's all together. Something like:
let message = String.localizedStringWithFormat(NSLocalizedString("We just emailed a link to %@. Please check your mail app and tap the link to log in.", comment: "blah"), emailAddress)
…ssage instead of combined strings
ScoutHarris
suggested changes
Feb 5, 2020
Contributor
ScoutHarris
left a comment
ScoutHarris
left a comment
There was a problem hiding this comment.
Hey @Gio2018 .
On a real iPad (not a simulator), when tapping the Open Mail button, it crashes:
2020-02-05 14:19:10:286 WordPress[5383:3237395] 🔵 Tracked: login_magic_link_open_email_client_viewed
2020-02-05 14:19:10.293523-0700 WordPress[5383:3237395] *** Terminating app due to uncaught exception 'NSGenericException', reason: 'Your application has presented a UIAlertController (<UIAlertController: 0x1170b8a00>) of style UIAlertControllerStyleActionSheet from WordPressAuthenticator.LoginNavigationController (<WordPressAuthenticator.LoginNavigationController: 0x116093a00>). The modalPresentationStyle of a UIAlertController with this style is UIModalPresentationPopover. You must provide location information for this popover through the alert controller's popoverPresentationController. You must provide either a sourceView and sourceRect or a barButtonItem. If this information is not known when you present the alert controller, you may provide it in the UIPopoverPresentationControllerDelegate method -prepareForPopoverPresentation.'
frosty
approved these changes
Feb 6, 2020
Contributor
frosty
left a comment
frosty
left a comment
There was a problem hiding this comment.
Looks good! Tested on iPad, and that problem seems fixed too 👍
ScoutHarris
approved these changes
Feb 6, 2020
Contributor
ScoutHarris
left a comment
ScoutHarris
left a comment
There was a problem hiding this comment.
Tested on iPad, and that problem seems fixed too
Ditto that. Thanks @Gio2018 !
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Magic Email Clients
Add AppSelector class, that creates an action sheet that contains a list of apps that can be called by tapping on its buttons. The specific use case are (magic) installed email clients
Add LinkMailPresenter, that presents the action sheet on a UIViewController. If no other client is available, Apple Mail will be opened directly without presenting the action sheet
Add the protocol URLHandler as a generic URL handler, injected into AppSelector. UIApplication conforms to URLHandler (and it's the default to handle URL Schemes)
Related WordPress-iOS PR: wordpress-mobile/WordPress-iOS#13275