-
Notifications
You must be signed in to change notification settings - Fork 247
browser: add locator.evaluate and evaluateHandle #2098
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
base: main
Are you sure you want to change the base?
Conversation
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.
Left a couple of non-blocking comment. Feel free to dismiss them if I missed an important context piece 🙇🏻
|
|
||
| # evaluate(pageFunction[, arg]) | ||
|
|
||
| Returns the value of the `pageFunction` invocation. It passes the matching element of the locator as the first argument to the `pageFunction` and arg as a second argument. |
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.
This information is super valuable, but felt really dry as I read it first. I suggest starting it with a high level overview of what this API will do for the user.
I see Playwright describes their evaluate method as "Execute JavaScript code in the page, taking the matching element as an argument.", which, from a user perspective answers the first question I had immediately: what is this API gonna do for me.
They only mention the pageFunction, what it does and how to use it later in a "details" section.
What do you think of:
| Returns the value of the `pageFunction` invocation. It passes the matching element of the locator as the first argument to the `pageFunction` and arg as a second argument. | |
| Execute JavaScript code in the page, taking the matching element as an argument. | |
| Returns the value of the `pageFunction` invocation. It passes the matching element of the locator as the first argument to the `pageFunction` and arg as a second argument. |
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.
I must admit I mostly copy/pasted the existing doc from other classes (as this function was already implemented in several places).
But it makes sense, I would rather suggest something like:
| Returns the value of the `pageFunction` invocation. It passes the matching element of the locator as the first argument to the `pageFunction` and arg as a second argument. | |
| Executes JavaScript code in the page, passing the matching element of the locator as the first argument to the `pageFunction` and arg as a second argument. It returns the value of the `pageFunction` invocation. |
WDYT @ankur22? If we agree on a function description, I'll update it in all pages and not only here.
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.
Yep, thanks for this! the updated wording LGTM 🚀
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.
I made the update to all pages 👍
docs/sources/k6/next/javascript-api/k6-browser/locator/evaluate.md
Outdated
Show resolved
Hide resolved
docs/sources/k6/next/javascript-api/k6-browser/locator/evaluatehandle.md
Outdated
Show resolved
Hide resolved
|
💻 Deploy preview available (browser: add locator.evaluate and evaluateHandle): |
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.
LGTM 🚀
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.
Left some minor nit suggestions, overall it looks great! 🙇
docs/sources/k6/next/javascript-api/k6-browser/jshandle/evaluate.md
Outdated
Show resolved
Hide resolved
docs/sources/k6/next/javascript-api/k6-browser/jshandle/evaluatehandle.md
Outdated
Show resolved
Hide resolved
docs/sources/k6/next/javascript-api/k6-browser/locator/evaluate.md
Outdated
Show resolved
Hide resolved
docs/sources/k6/next/javascript-api/k6-browser/locator/evaluatehandle.md
Outdated
Show resolved
Hide resolved
docs/sources/k6/next/javascript-api/k6-browser/locator/evaluate.md
Outdated
Show resolved
Hide resolved
docs/sources/k6/next/javascript-api/k6-browser/locator/evaluatehandle.md
Outdated
Show resolved
Hide resolved
docs/sources/k6/next/javascript-api/k6-browser/locator/evaluatehandle.md
Outdated
Show resolved
Hide resolved
|
This is unrelated to the changes in the PR, but seems like the Run code blocks workflow is failing on Looks like we need to change the line 2:
To:
|
docs/sources/k6/next/javascript-api/k6-browser/jshandle/evaluatehandle.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Heitor Tashiro Sergent <heitortsergent@gmail.com>
What?
This PR adds the following functions in the
locatorAPI documentation:Checklist
npm startcommand locally and verified that the changes look good.Related PR(s)/Issue(s)
Implementation PR: grafana/k6#5306