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

Re-enable screenshots for iPad #6403

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Conversation

niklasberglund
Copy link
Contributor

@niklasberglund niklasberglund commented Jun 25, 2024

Re-enable screenshots for iPad, since the iPad specific UI is now gone and screenshot tests should work on iPad again. The changes can be tested by running the iOS create screenshots workflow on this branch.

Also updated configuration so that HAS_TIME_ACCOUNT_NUMBER is properly set and partner API is not used.


This change is Reviewable

@niklasberglund niklasberglund added the iOS Issues related to iOS label Jun 25, 2024
@niklasberglund niklasberglund force-pushed the re-enable-screenshots-for-ipad branch from 44a53fc to 7eab5d6 Compare June 25, 2024 10:51
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 1 of 1 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@niklasberglund niklasberglund force-pushed the re-enable-screenshots-for-ipad branch from 7eab5d6 to 92a1a22 Compare June 26, 2024 14:33
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 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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


ios/MullvadVPNUITests/Pages/Page.swift line 53 at r3 (raw file):

    @discardableResult func tapWhereStatusBarShouldBeToScrollToTopMostPosition() -> Self {
        app.coordinate(withNormalizedOffset: CGVector(dx: 0.75, dy: 0)).tap()

Was there a specific reason for this change ?

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.

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


ios/MullvadVPNUITests/Pages/Page.swift line 53 at r3 (raw file):

Previously, buggmagnet wrote…

Was there a specific reason for this change ?

On iPad there's a button in the middle of the nav bar to handle split screen and such. Touching that area on a phone makes the current scroll view scroll to top, but touching the same area results in a click in the button instead. Which fails the test as it cannot proceed. Offsetting the touch a little "misses" the button and yields the result we want. It can probably be considered a hack, but if so the original implementation is as well.

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


ios/MullvadVPNUITests/Pages/Page.swift line 53 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

On iPad there's a button in the middle of the nav bar to handle split screen and such. Touching that area on a phone makes the current scroll view scroll to top, but touching the same area results in a click in the button instead. Which fails the test as it cannot proceed. Offsetting the touch a little "misses" the button and yields the result we want. It can probably be considered a hack, but if so the original implementation is as well.

That's totally fine, we should document it though, to make sure our future selves do not forget, because it looks like a magic value from an ignorant observer's point of view.

@niklasberglund niklasberglund force-pushed the re-enable-screenshots-for-ipad branch from 92a1a22 to de35942 Compare June 27, 2024 11:39
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 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@buggmagnet buggmagnet force-pushed the re-enable-screenshots-for-ipad branch from de35942 to bb89790 Compare June 27, 2024 11:41
@buggmagnet buggmagnet merged commit 94c1da4 into main Jun 27, 2024
8 checks passed
@buggmagnet buggmagnet deleted the re-enable-screenshots-for-ipad branch June 27, 2024 11:42
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