Skip to content

Test PR to verify isFullyVisibleOnScreen removal#17628

Closed
mokagio wants to merge 2 commits intodevelopfrom
is-fully-visible-on-screen
Closed

Test PR to verify isFullyVisibleOnScreen removal#17628
mokagio wants to merge 2 commits intodevelopfrom
is-fully-visible-on-screen

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented Dec 6, 2021

No description provided.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 6, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 6, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

var iteration = 0

while !isFullyVisibleOnScreen && iteration < threshold {
while isFullyVisibleOnScreen() && iteration < threshold {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is the test PR, but was this logic change intentional? !isFullyVisibleOnScreen -> isFullyVisibleOnScreen()

I would expect some tests failing because of that, but I remember seeing cases where isFullyVisibleOnScreen returned true for an element that was partially visible. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch @tiagomar! This is purely a mistake on my end.

This actually made me smile because I left a comment earlier on !foo vs foo == false and how ! can be missed, which is exactly the error I made here. Case in point, I guess 🙃

I will fix it anyway, because I plan to cherrypick this commit in the PR that will update XCUITestHelper to a the new version once HACK week is done 👍

@mokagio mokagio force-pushed the is-fully-visible-on-screen branch from 937369e to e71a109 Compare December 7, 2021 05:36
@mokagio
Copy link
Contributor Author

mokagio commented Dec 20, 2021

Closing in favor of #17691

@mokagio mokagio closed this Dec 20, 2021
@mokagio mokagio deleted the is-fully-visible-on-screen branch December 20, 2021 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants