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

feat: Add visible options to toMatchElement config #208

Merged
merged 1 commit into from
Mar 13, 2019

Conversation

xiaoyuhen
Copy link
Contributor

@xiaoyuhen xiaoyuhen commented Mar 2, 2019

Summary

Add visible options to toMatchElement config,.
wait for element to be present in DOM and to be visible, i.e. to not have display: none or visibility: hidden CSS properties. Defaults to false.

see #207

Test plan

see test case

@xiaoyuhen xiaoyuhen requested a review from gregberge March 2, 2019 16:10
Copy link
Member

@gregberge gregberge left a comment

Choose a reason for hiding this comment

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

I think it is pretty "fragile", we don't have another option to detect if an element is visible or not?

packages/expect-puppeteer/src/matchers/toMatchElement.js Outdated Show resolved Hide resolved
@xiaoyuhen
Copy link
Contributor Author

@neoziro

use selector is an easy way, but If we want to check the element not indeed appear, check that display and visibility are incomplete.

I will confirm if puppeteer will detect height === 0 and then determine how we do

@gregberge
Copy link
Member

@xiaoyuhen does it work when the element has a class that has a "display: none"? If not, then it is not usable.

@xiaoyuhen
Copy link
Contributor Author

@neoziro

Yeah, it works. i update the test case.

@Aaron-Pool
Copy link

Have you guys considered using the Intersection Observer API for this case?

https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API

@gregberge
Copy link
Member

@Aaron-Pool why not, if it works better, it is good for me.

@Aaron-Pool
Copy link

@neoziro I can't say with 100% certainty that it's "better". But I have used it before, and it seems pretty reliable and performant. Also, considering the MDN docs start with "Historically, detecting visibility of an element [...] has been a difficult task for which solutions have been unreliable", it seems like it was introduced for purposes just like these 👍

@Aaron-Pool
Copy link

Aaron-Pool commented Mar 7, 2019

@neoziro @xiaoyuhen Actually, the more I look at it, I'm curious as to why you guys decided to use page.waitForFunction in the toMatchElement function, rather than page.waitForSelector. If you used page.waitForSelector this functionality would be free, because the puppeteer team has already implemented it (see here)

You guys might have a perfectly good reason for doing so, just curious what it was 👍

@gregberge
Copy link
Member

@xiaoyuhen tests are not passing, could you fix it please?

@gregberge
Copy link
Member

@xiaoyuhen I think the class strategy does not work

@xiaoyuhen
Copy link
Contributor Author

@neoziro
Yeah, you are right. It doesn't work. will use another way to solve this problem later

@xiaoyuhen
Copy link
Contributor Author

@Aaron-Pool

thanks for your suggestion, we cant use page.waitForSelector, because we want to match a NodeList.

Intersection Observer API is a good solution, but it's too heavy, I choose the way same with puppeteer.

see https://github.com/GoogleChrome/puppeteer/blob/a064a6341b9ec19aef9253db1d79d228bd9018c1/lib/DOMWorld.js#L510

@xiaoyuhen
Copy link
Contributor Author

xiaoyuhen commented Mar 13, 2019

@neoziro

it looks like works well now, could you please review this pr again?

@gregberge
Copy link
Member

Good for me thanks!

@gregberge gregberge merged commit 46d8ec1 into argos-ci:master Mar 13, 2019
@xiaoyuhen xiaoyuhen deleted the matchElement branch March 24, 2019 08:15
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.

3 participants