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

Add info modals to default access methods screens #6905

Merged

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Oct 2, 2024

Changes were made to the info text for the Encrypted DNS proxy, where one of the providers was written incorrectly. Also, to show this info text, new headers have been added to each default access method screen.


This change is Reviewable

@rablador rablador added the iOS Issues related to iOS label Oct 2, 2024
@rablador rablador self-assigned this Oct 2, 2024
Copy link

linear bot commented Oct 2, 2024

@rablador rablador force-pushed the change-encrypted-dns-proxy-info-text-copy-ios-849 branch 2 times, most recently from faae446 to e5f3ecf Compare October 2, 2024 10:18
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

Reviewed 14 of 15 files at r1, all commit messages.
Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel.swift line 90 at r1 (raw file):

                    "DIRECT_ACCESS_METHOD_HEADER_LINK",
                    tableName: "APIAccess",
                    value: "About Direct method...",

It kinda reads weird like this, it should probably be "About the direct method..."
Or if we go the Apple way, we just put "Learn more..."


ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel.swift line 227 at r1 (raw file):

                        If you are not connected to our VPN, then the Encrypted DNS proxy will use your own non-VPN IP \
                        when connecting.
                        The DoH servers are hosted by one of the following providers: Quad 9, CloudFlare, or Google.

"Cloudflare" (no uppercase F)
"Quad 9" should not have a space between Quad and 9

Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel.swift line 90 at r1 (raw file):

Previously, buggmagnet wrote…

It kinda reads weird like this, it should probably be "About the direct method..."
Or if we go the Apple way, we just put "Learn more..."

I agree, but I have followed the copy in Figma.


ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel.swift line 227 at r1 (raw file):

Previously, buggmagnet wrote…

"Cloudflare" (no uppercase F)
"Quad 9" should not have a space between Quad and 9

have followed the copy in Figma, but I guess we can change pure naming errors?

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel.swift line 90 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I agree, but I have followed the copy in Figma.

We should follow Figma here.


ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel.swift line 227 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

have followed the copy in Figma, but I guess we can change pure naming errors?

Nope, it should be Quad9 and Cloudflare.

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions


ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel.swift line 227 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

have followed the copy in Figma, but I guess we can change pure naming errors?

Well Figma is wrong, so we need to update it there too ( :

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel.swift line 227 at r1 (raw file):

Previously, buggmagnet wrote…

Well Figma is wrong, so we need to update it there too ( :

Matilda has been notified.

Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel.swift line 90 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

We should follow Figma here.

Done.


ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel.swift line 227 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Matilda has been notified.

Done. And to clarify, I agree that we should change to the correct names.

@rablador rablador force-pushed the change-encrypted-dns-proxy-info-text-copy-ios-849 branch from e5f3ecf to ef6126d Compare October 2, 2024 12:13
@pinkisemils pinkisemils force-pushed the change-encrypted-dns-proxy-info-text-copy-ios-849 branch from ef6126d to 47a0834 Compare October 2, 2024 12:17
Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)

@pinkisemils pinkisemils merged commit 4fc7824 into main Oct 2, 2024
8 of 9 checks passed
@pinkisemils pinkisemils deleted the change-encrypted-dns-proxy-info-text-copy-ios-849 branch October 2, 2024 12:26
Copy link

github-actions bot commented Oct 2, 2024

🚨 End to end tests failed. Please check the failed workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants