-
Notifications
You must be signed in to change notification settings - Fork 384
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
Conversation
44a53fc
to
7eab5d6
Compare
There was a problem hiding this 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:complete! all files reviewed, all discussions resolved
7eab5d6
to
92a1a22
Compare
There was a problem hiding this 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:complete! all files reviewed, all discussions resolved
There was a problem hiding this 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 ?
There was a problem hiding this 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.
There was a problem hiding this 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.
92a1a22
to
de35942
Compare
There was a problem hiding this 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 r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
de35942
to
bb89790
Compare
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