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

[ios] Add default email client support to about menu #7862

Merged
merged 5 commits into from
Apr 24, 2024

Conversation

v-lozko
Copy link
Contributor

@v-lozko v-lozko commented Apr 8, 2024

These changes are to add default email client support in the about menu. Issue #7758.

I tested it on my ipad 11 running 17.0.3

I would post a video but opening the email app does share some personal info

Signed-off-by: Valery Lozko <valerylozko@gmail.com>
Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

Thanks! Did you test it with other mail clients? GMail, Proton for example?

@kirylkaveryn PTAL

@kirylkaveryn kirylkaveryn self-requested a review April 8, 2024 07:33
@kirylkaveryn
Copy link
Contributor

kirylkaveryn commented Apr 8, 2024

Please provide some screenshots (with erased personal info) of the results when you pass one subject or an array of subjects. And with different apps as @biodranik mentioned before.

@matheusgomesms
Copy link
Contributor

Could you please check with Outlook as well? :)

Signed-off-by: Valery Lozko <valerylozko@gmail.com>
@v-lozko
Copy link
Contributor Author

v-lozko commented Apr 9, 2024

I made edits to the code based on suggestions

Here is gmail:
Gmail

here is apple native app:
apple

here is outlook:
outlook

@v-lozko
Copy link
Contributor Author

v-lozko commented Apr 9, 2024

EDIT: I wasn't testing it appropriately, it works fine with semilcolon:

image

@kirylkaveryn I was just testing with multiple individuals on the email, and is the ";" most appropriate seperator? it doesn't seem to add multiple individuals to the 'to' line appropriately. Should the seperator be a comma instead?

Here is it with commas:
Screenshot 2024-04-09 at 9 07 48 AM

using the ';' made the recipient a single line

@kirylkaveryn
Copy link
Contributor

kirylkaveryn commented Apr 9, 2024

@kirylkaveryn I was just testing with multiple individuals on the email, and is the ";" most appropriate seperator? it doesn't seem to add multiple individuals to the 'to' line appropriately. Should the seperator be a comma instead?

You're right! Coma is the correct symbol for the recipients joining.
I've replace ; with , and tested your branch with Spark and it works well. Also, I've tested on the ios 12.5.
So let's use your version with coma.

Thanks!

@v-lozko
Copy link
Contributor Author

v-lozko commented Apr 18, 2024

@biodranik please let me know if you need anything else for this.

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

LGTM

iphone/Maps/UI/Help/AboutController/AboutController.swift Outdated Show resolved Hide resolved
Signed-off-by: Valery Lozko <valerylozko@gmail.com>
@kirylkaveryn
Copy link
Contributor

kirylkaveryn commented Apr 20, 2024

@biodranik I think we can merge this one!
@v-lozko thanks a lot!

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

Thanks! So the only case when user will see the error dialog is when no any mail client is installed, right? Does it work properly on iOS 12 too?

iphone/Maps/UI/Help/AboutController/AboutController.swift Outdated Show resolved Hide resolved
@kirylkaveryn
Copy link
Contributor

kirylkaveryn commented Apr 20, 2024

Thanks! So the only case when user will see the error dialog is when no any mail client is installed, right? Does it work properly on iOS 12 too?

I've tested on iOS 12.5 and it works well.

Signed-off-by: Valery Lozko <valerylozko@gmail.com>
@v-lozko
Copy link
Contributor Author

v-lozko commented Apr 23, 2024

@biodranik let me know if there's anything else. I addressed all the comments that I saw.

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

Thanks! A minor nit )

iphone/Maps/UI/Help/AboutController/AboutController.swift Outdated Show resolved Hide resolved
Co-authored-by: Alexander Borsuk <170263+biodranik@users.noreply.github.com>
Signed-off-by: v-lozko <156805389+v-lozko@users.noreply.github.com>
@biodranik biodranik merged commit 8d8964c into organicmaps:master Apr 24, 2024
5 checks passed
kavikhalique pushed a commit to kavikhalique/organicmaps that referenced this pull request May 25, 2024
* [ios] Add default email client support to about menu

Signed-off-by: Valery Lozko <valerylozko@gmail.com>
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.

None yet

4 participants