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

xk6-browser API documentation #642

Closed
wants to merge 13 commits into from

Conversation

inancgumus
Copy link
Member

No description provided.

@inancgumus
Copy link
Member Author

inancgumus commented Apr 26, 2022

@imiric, Could you please take a look at my initial commits? I'll keep doing it like that if it's fine.

At this moment, a thorough review is unnecessary, and I don't want to take much of your time. I only need to hear from you that the formatting is good, and we can keep doing it like that :)

PS: The text is a direct copy from the Playwright's documentation.

@inancgumus inancgumus force-pushed the docs/582-xk6-browser branch 2 times, most recently from e11bbc7 to 46b9e04 Compare April 26, 2022 13:44
@imiric imiric changed the base branch from main to xk6-browser-new-format April 26, 2022 14:18
@inancgumus inancgumus force-pushed the docs/582-xk6-browser branch from 46b9e04 to 2fdc597 Compare April 26, 2022 15:19
@inancgumus
Copy link
Member Author

inancgumus commented Apr 26, 2022

@imiric, I couldn't run it locally using npm run develop even if I use the xk6-browser-new-format branch (page 404 error and other weird errors pop up). However, everything works fine with the main branch. Please let me know how you could run it locally for the xk6-browser-new-format branch.

I tried a lot of stuff. Deleting the node_modules directory (locally and globally), npm run clean, and others. Setting up a development environment file, etc. But no luck.

@imiric
Copy link
Contributor

imiric commented Apr 26, 2022

I didn't do anything special. Just:

$ rm -rf node_modules
$ npm i
$ npm start

I'm using Node v16.14.2 and npm v8.5.0.

On xk6-browser-new-format I get some warnings, but ultimately get success createPages and no 404. On this branch I get failed createPages with the same error as in CI and the 404.

And also another error which I think is what's making createPages fail:

 ERROR #11321  PLUGIN

"gatsby-node.js" threw an error while running the createPages lifecycle:

Cannot read properties of undefined (reading 'frontmatter')

  240 |         shouldCreatePage,
  241 |       },
> 242 |     } = remarkNode;
      |         ^
  243 |
  244 |     // skip md files that we don't generate pages for
  245 |     if (shouldCreatePage === false) {

The Markdown here is very brittle and it's likely some formatting change broke it. 😞 I'll take a look.

Unfortunately, links in h3s are not supported :-/
@imiric imiric force-pushed the docs/582-xk6-browser branch from 8b339ed to a9c70c7 Compare April 26, 2022 16:34
@imiric
Copy link
Contributor

imiric commented Apr 26, 2022

I pushed a "fix" for createPages and, strangely enough, I don't get the 404s anymore, but the Browser page wasn't loading because of an obscure Error in function slugify in ./src/utils/utils.js:42... Gatsby errors are practically useless. 🤦‍♂️

The cause were the links in h3s, so we'll have to find another way of doing this, or ask Pepe or PixelPoint to help us add support for it.

@inancgumus
Copy link
Member Author

inancgumus commented Apr 27, 2022

@imiric, thanks for looking into this. Now it works!

I created links for h3s right before them, which did the trick (7a49dd2 and d54b50d). I kept the first one invisible to see how it behaves. Let me know if we should continue on this route. Visible or invisible doesn't seem to make a difference so we better keep them visible (preventing style pollution).

Regarding red-x (not implemented), I added ❌ with a tooltip. The tooltip content is global, and we can use it for other stuff in the same markdown file.

Please take a look at the commit 4c7d4da and let me know WDYT.


One thing I noticed is that the Browser page (and also others) can be very lengthy and crowded. We better provide a list of methods first on a page and then have a page for each method. For example, we can list the methods in a table on the Browser page, and then people can click on the links to see each method such as browser.newContext.

As in here: https://k6.io/docs/javascript-api/k6-html/selection/. The selectors are on a single page and then each link navigates to another page.

@inancgumus inancgumus requested a review from imiric April 27, 2022 08:34
@inancgumus
Copy link
Member Author

@imiric, by the way, let's copy-paste Playwright descriptions first. We can rephrase them after we finish. You don't have to follow the same route, though :) I want to work in this way as the process is already complicated (Gatsby etc.).

- Finish the browser.newContext list
- Started on browser.newPage
+ Removes descriptions of missing APIs (See:
  #508 (comment))
+ Formats input and return value tables for all.
+ Adds a link for browser.close so that the link in browser.on
  description works.
+ Updates browser.on() to browser.on('disconnected') per Playwright docs
@imiric
Copy link
Contributor

imiric commented Apr 27, 2022

let's copy-paste Playwright descriptions first

That feels like we'd be doing double the work: first to copy things and then to rephrase. Which would involve 2 review passes as well. What should I review on the first pass? Should I ignore if the Playwright docs are wrong for us, as in this case?

It wouldn't be much more work to paraphrase their docs or come up with our own docs from the start, and we can avoid double reviews. It will be a lot of content to go through anyway...

Let's first agree on the layout changes and do the final content after that.

One thing I noticed is that the Browser page (and also others) can be very lengthy and crowded. We better provide a list of methods first on a page and then have a page for each method.

Yeah, that might be better. We probably don't need it for every object, so let's decide on a case by case basis whether it needs splitting? In the case that we do split it, let's remove the sticky right-side menu.

@imiric
Copy link
Contributor

imiric commented Apr 27, 2022

Re: the tooltip, it looks great in Firefox, but it doesn't show up for me in Chromium v99:

2022-04-27-151315_492x88_scrot

Maybe something with the SVG?

One thing that's a bit confusing is that it's an empty link. It should only be a link if there's an associated GitHub issue we can link to. I guess we can hack it to not look and behave like a link if it's empty, but it feels a bit wonky. Especially since clicking on it changes the URL to http://localhost:8000/javascript-api/xk6-browser/browser/##, and then refreshing the page shows this:

2022-04-27-152650_1271x814_scrot

BTW, is that a generic Markdown feature or Gatsby's?

@inancgumus
Copy link
Member Author

inancgumus commented Apr 27, 2022

let's copy-paste Playwright descriptions first

That feels like we'd be doing double the work: first to copy things and then to rephrase. Which would involve 2 review passes as well. What should I review on the first pass? Should I ignore if the Playwright docs are wrong for us, as in this case?

It wouldn't be much more work to paraphrase their docs or come up with our own docs from the start, and we can avoid double reviews. It will be a lot of content to go through anyway...

I tentatively agree (on 2 review passes—but it often happens anyway). However, I don't think it would be a lot of time-consuming work. You can first check out the types, methods, return types, etc. without looking at the descriptions (we can leave them empty as well). Then, the next commit will only show the paraphrased descriptions, and you won't go through the same process of checking the other stuff. I'm suggesting this because I feel kind of overwhelmed here.

Let's first agree on the layout changes and do the final content after that.

That's basically what I've been trying to say :) Let's first add the types, methods, properties, and agree on the layout. Make a commit. Then, paraphrase in the next commit.

One thing I noticed is that the Browser page (and also others) can be very lengthy and crowded. We better provide a list of methods first on a page and then have a page for each method.

Yeah, that might be better. We probably don't need it for every object, so let's decide on a case by case basis whether it needs splitting? In the case that we do split it, let's remove the sticky right-side menu.

Wouldn't the content look weird if we do it on a case-by-case basis? I mean, people would need to go to a different page for some and stay on the same page for others. But I understand that some of them would be really weird (with a little content on a whole page, yeah). We can try doing it for the Browser type and then see what it looks like to decide which way to go. WDYT?

@inancgumus
Copy link
Member Author

inancgumus commented Apr 27, 2022

Re: the tooltip, it looks great in Firefox, but it doesn't show up for me in Chromium v99:

I'm using Chrome v100 and Safari 15.4, and it works fine.

Maybe something with the SVG?

I wish I had an idea 🤷

One thing that's a bit confusing is that it's an empty link. It should only be a link if there's an associated GitHub issue we can link to. I guess we can hack it to not look and behave like a link if it's empty, but it feels a bit wonky.

Yeah, I know, but let me know if you have another idea cause I couldn't find another one (another technical trick). It's hacky-wacky stuff of Markdown itself.

Especially since clicking on it changes the URL to http://localhost:8000/javascript-api/xk6-browser/browser/##, and then refreshing the page shows this.

Wow, it crashed Gatsby when I did so! Weird.

@inancgumus
Copy link
Member Author

I created links for h3s right before them, which did the trick (7a49dd2 and d54b50d). I kept the first one invisible to see how it behaves. Let me know if we should continue on this route. Visible or invisible doesn't seem to make a difference so we better keep them visible (preventing style pollution).

@imiric, WDYT about this one?

@imiric
Copy link
Contributor

imiric commented Apr 27, 2022

That's basically what I've been trying to say

No, I mean let's agree on a layout template; how to implement the tooltip, split pages or not, other formatting, etc. And then start working on the final version of the content.

But OK, if you find it easier this way, we'll handle it.

BTW, can we use this PR to agree on the layout changes and then close it? I don't want to get a GH notification everytime you push to this branch, and viceversa. We can discuss anything else quicker over Slack anyway.

Wouldn't the content look weird if we do it on a case-by-case basis?

I think it would look weirder if we had split pages for small objects like Video. I don't see a point in sticking to a strict layout for consistency's sake. But sure, let's see how it looks and decide later.

Let's try to think of a different way of implementing the tooltip. I'd rather have to repeat it everywhere then deal with more hacks. I'll experiment with something on the BrowserContext page as well.

Re: invisible links, we don't really need them. All h3s have an id attribute, so fragment links already work. E.g. for "geolocation option", you can link to it with #geolocation-option.

@inancgumus inancgumus force-pushed the docs/582-xk6-browser branch 4 times, most recently from 21511a9 to 1181f8c Compare April 28, 2022 11:00
@inancgumus inancgumus force-pushed the docs/582-xk6-browser branch from 1181f8c to 0134e44 Compare April 28, 2022 11:01
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM, besides the minor comments here.

The x and tooltip looks good. I'm assuming you'll split this page later, right?

Can we close this PR since we mostly agree on the layout changes? We can merge the branch later into xk6-browser-new-format once we make more progress.

As discussed on Slack, methods with arguments don't work.
For example:
+ browser.on('disconnected') fails the Gatsby server.
+ browser.newContext([options]) has an invalid link on the left
  navigation menu.
+ browser.newPage is similar to newContext.

We need to figure out how to move forward with these problems.
@inancgumus inancgumus requested a review from imiric April 28, 2022 12:57
@inancgumus
Copy link
Member Author

inancgumus commented Apr 28, 2022

Can we close this PR since we mostly agree on the layout changes? We can merge the branch later into xk6-browser-new-format once we make more progress.

Let's wait until you make a final review of the new page structure (subpages). Then we can close it.

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM

@inancgumus inancgumus closed this Apr 28, 2022
ankur22 pushed a commit that referenced this pull request Jul 12, 2022
ankur22 pushed a commit that referenced this pull request Jul 14, 2022
ankur22 pushed a commit that referenced this pull request Jul 27, 2022
ankur22 pushed a commit that referenced this pull request Aug 2, 2022
ankur22 pushed a commit that referenced this pull request Aug 4, 2022
ankur22 pushed a commit that referenced this pull request Aug 4, 2022
ankur22 pushed a commit that referenced this pull request Aug 4, 2022
ankur22 pushed a commit that referenced this pull request Sep 6, 2022
ankur22 pushed a commit that referenced this pull request Sep 8, 2022
@inancgumus inancgumus added the Area: browser The browser module label Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: browser The browser module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants