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 the openPortSelectorMenuButton accessibility identifier to ... buttons #7282

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

acb-mv
Copy link
Contributor

@acb-mv acb-mv commented Dec 4, 2024

... in VPN Settings

This creates a new AccessibilityIdentifier named .openPortSelectorMenuButton and sets the "..." button, for calling up the port selector menu in obfuscation method cells, to have it. It also refactors VPNSettingsPage, combining the logic for tapping disclosure cells with that for tapping this button.


This change is Reviewable

@acb-mv acb-mv added the iOS Issues related to iOS label Dec 4, 2024
@acb-mv acb-mv self-assigned this Dec 4, 2024
@acb-mv acb-mv force-pushed the add-OpenPortSelectorMenu-accessibility-identifier branch from 47d9d3c to 2453ed0 Compare December 4, 2024 15:52
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.

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @acb-mv)


ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 62 at r1 (raw file):

    }

    @discardableResult func tapUDPOverTCPPortSelectorButton() -> Self {

these should be revoked instead of the former ones


ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 74 at r1 (raw file):

    }

    // this button no longer exists

these codes and usages should be removed from here and SettingsMigrationTests


ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 82 at r1 (raw file):

    // this button no longer exists
    @discardableResult func tapUDPOverTCPPortAutomaticCell() -> Self {

same above


ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 88 at r1 (raw file):

    }

    // this button no longer exists

same above


ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 95 at r1 (raw file):

    }

    // this button no longer exists

same above

Copy link
Contributor Author

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

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @mojganii)


ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 62 at r1 (raw file):

Previously, mojganii wrote…

these should be revoked instead of the former ones

I will do so in a subsequent PR.


ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 74 at r1 (raw file):

Previously, mojganii wrote…

these codes and usages should be removed from here and SettingsMigrationTests

They will be in a subsequent PR; I am keeping the old tests which no longer work to crib from for writing the new tests.


ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 82 at r1 (raw file):

Previously, mojganii wrote…

same above

Noted


ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 88 at r1 (raw file):

Previously, mojganii wrote…

same above

Noted


ios/MullvadVPNUITests/Pages/VPNSettingsPage.swift line 95 at r1 (raw file):

Previously, mojganii wrote…

same above

Noted

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: 0 of 3 files reviewed, 5 unresolved discussions

Copy link
Contributor

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mojganii)

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 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mojganii)

@buggmagnet buggmagnet force-pushed the add-OpenPortSelectorMenu-accessibility-identifier branch from 2453ed0 to ccaf4ed Compare December 6, 2024 08:10
@buggmagnet buggmagnet merged commit 4baba02 into main Dec 6, 2024
10 of 11 checks passed
@buggmagnet buggmagnet deleted the add-OpenPortSelectorMenu-accessibility-identifier branch December 6, 2024 08:13
Copy link

github-actions bot commented Dec 6, 2024

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

Copy link

github-actions bot commented Dec 6, 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.

4 participants