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 a new container view to wrap the whole location selection #6181

Merged

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Apr 25, 2024

The wrapper should have a segmented control. The child will be updated through the wrapper, keeping most of the previous implementation intact.


This change is Reviewable

@rablador rablador added the iOS Issues related to iOS label Apr 25, 2024
Copy link

linear bot commented Apr 25, 2024

@rablador rablador force-pushed the add-a-new-container-view-to-wrap-the-whole-location-ios-630 branch from e5f6f2d to 7caa345 Compare April 25, 2024 10:45
@rablador rablador requested review from mojganii and buggmagnet April 25, 2024 11:58
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 3 of 7 files at r1, all commit messages.
Reviewable status: 3 of 7 files reviewed, 2 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/View controllers/SelectLocation/LocationViewControllerWrapper.swift line 66 at r1 (raw file):

        navigationItem.largeTitleDisplayMode = .never

        navigationItem.title = NSLocalizedString(

We will probably want to have accessibility on those items if we don't want to break already existing UI Tests here.
Please Make sure you run the Custom Lists UI tests when you rebase from main.


ios/MullvadVPN/View controllers/SelectLocation/LocationViewControllerWrapper.swift line 102 at r1 (raw file):

        segmentedControl.insertSegment(withTitle: NSLocalizedString(
            "SEGMENTED_CONTROL_ENTRY",

nit
Typically the key parameter of a localised string is used by translators to figure out the context.
Here, "segmented control" doesn't mean anything to a non engineer.
Also it doesn't really matter that it's a segmented control, it could be anything.
A more fitting name would probably be "MULTIHOP_LABEL_ENTRY" or something similar, that is more descriptive as to which element the localization pertains.

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: 3 of 7 files reviewed, 1 unresolved discussion (waiting on @buggmagnet)


ios/MullvadVPN/View controllers/SelectLocation/LocationViewControllerWrapper.swift line 66 at r1 (raw file):

Previously, buggmagnet wrote…

We will probably want to have accessibility on those items if we don't want to break already existing UI Tests here.
Please Make sure you run the Custom Lists UI tests when you rebase from main.

ID was added in main and custom list ui tests pass now.


ios/MullvadVPN/View controllers/SelectLocation/LocationViewControllerWrapper.swift line 102 at r1 (raw file):

Previously, buggmagnet wrote…

nit
Typically the key parameter of a localised string is used by translators to figure out the context.
Here, "segmented control" doesn't mean anything to a non engineer.
Also it doesn't really matter that it's a segmented control, it could be anything.
A more fitting name would probably be "MULTIHOP_LABEL_ENTRY" or something similar, that is more descriptive as to which element the localization pertains.

Done.

@rablador rablador force-pushed the add-a-new-container-view-to-wrap-the-whole-location-ios-630 branch 2 times, most recently from ab0cb28 to 1d83511 Compare May 6, 2024 12:16
Copy link
Contributor

@acb-mv acb-mv 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 7 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

while these changes aim to be merged into main, then we should show [select location view] with the previous design instead of the changes are coming with multi-hop like segment.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

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.

:lgtm:

Reviewed 2 of 7 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@rablador rablador force-pushed the add-a-new-container-view-to-wrap-the-whole-location-ios-630 branch from 1d83511 to 72fb2a4 Compare May 13, 2024 07:25
@rablador rablador force-pushed the add-a-new-container-view-to-wrap-the-whole-location-ios-630 branch from 72fb2a4 to 2ad27f5 Compare May 13, 2024 07:26
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.

@mojganii If you run in mock release the segmented control will be hidden.

Reviewable status: 4 of 7 files reviewed, all discussions resolved (waiting on @acb-mv and @buggmagnet)

Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 4 of 7 files reviewed, all discussions resolved (waiting on @acb-mv and @buggmagnet)

@buggmagnet buggmagnet merged commit f91ba00 into main May 13, 2024
6 of 7 checks passed
@buggmagnet buggmagnet deleted the add-a-new-container-view-to-wrap-the-whole-location-ios-630 branch May 13, 2024 08:14
Copy link

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

4 participants