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(FrameManage): add title for WaitTask #2292

Merged
merged 1 commit into from Mar 30, 2018

Conversation

yanivefraim
Copy link
Contributor

@yanivefraim yanivefraim commented Mar 30, 2018

Closes #2037
(no answer from @felixfbecker regarding #2084. Created a new PR. Sorry for that...)

@@ -696,7 +696,8 @@ class Frame {
const waitForVisible = !!options.visible;
const waitForHidden = !!options.hidden;
const polling = waitForVisible || waitForHidden ? 'raf' : 'mutation';
return this.waitForFunction(predicate, {timeout: options.timeout, polling}, selectorOrXPath, isXPath, waitForVisible, waitForHidden);
const timeout = helper.isNumber(options.timeout) ? options.timeout : 30000;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a drive-by? Looks like an unrelated bugfix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it is related to this comment:

#2084 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Your comment link didn't work for me.

But if I understand correctly, the page.waitForSelector/page.waitForXPath don't currently have default timeout 30 seconds (despite documentation claiming so), and this change brings it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, created it from mobile (it is working on mobile... strange)

The comment was: #2084 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see now. Thank you for the explanation.

Copy link
Contributor

@aslushnikov aslushnikov left a comment

Choose a reason for hiding this comment

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

Nice patch

@aslushnikov aslushnikov merged commit dde45fa into puppeteer:master Mar 30, 2018
@felixfbecker
Copy link

Thank you!

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.

None yet

3 participants