Skip to content

Update to Async Mouse #1594

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 8 commits into from
May 22, 2024
Merged

Update to Async Mouse #1594

merged 8 commits into from
May 22, 2024

Conversation

inancgumus
Copy link
Member

What?

Update relevant Mouse methods from sync to async.

Checklist

  • I have used a meaningful title for the PR.
  • I have described the changes I've made in the "What?" section above.
  • I have performed a self-review of my changes.
  • I have run the npm start command locally and verified that the changes look good.
  • I have made my changes in the docs/sources/next folder of the documentation.
  • I have reflected my changes in the docs/sources/v{most_recent_release} folder of the documentation.
  • I have reflected my changes in the relevant folders of the two previous k6 versions of the documentation (if still applicable to previous versions).

Related PR(s)/Issue(s)

Updates: grafana/xk6-browser#1306

Closes: grafana/xk6-browser#766

@inancgumus inancgumus added the Area: browser The browser module label May 20, 2024
@inancgumus inancgumus self-assigned this May 20, 2024
@inancgumus inancgumus marked this pull request as ready for review May 20, 2024 12:20
@inancgumus inancgumus requested review from ankur22 and allansson May 20, 2024 12:20
@inancgumus inancgumus force-pushed the async/mouse branch 2 times, most recently from 74f05b2 to 1eaab62 Compare May 20, 2024 12:31
Copy link
Contributor

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

LGTM.

A bit of confusion for me on why mouseMove is also mentioned in mouseDown and mouseUp.


# down([options])

Dispatches a `mousedown` event on the mouse's current position. Mouse can be moved to a different position using [`mouse.move()`](https://grafana.com/docs/k6/<K6_VERSION>/javascript-api/k6-experimental/browser/mouse/move).
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to elaborate a little why a user might want to downClick and move (dragging an element i assume)? Otherwise it doesn't make sense to mention mouseMove here.

Copy link
Member Author

@inancgumus inancgumus May 21, 2024

Choose a reason for hiding this comment

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

It's related to this issue. Previously, down and up were accepting x and y coordinates, which was incorrect.

down and up are closely related to move because without a move, users will only be able to click (well, down or up) on the same part of the virtual screen.

As tested here, so, they first move and then use one of down or up to click to the x and y coordinates. That's why move is related.

But I can expand the doc's explanation similar to what I've done above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I retyped it like so 128c37e. It might be "slightly" better why these APIs are related.


# up([options])

Dispatches a `mouseup` event on the mouse's current position. Mouse can be moved to a different position using [`mouse.move()`](https://grafana.com/docs/k6/<K6_VERSION>/javascript-api/k6-experimental/browser/mouse/move).
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it makes sense to mention mouseMove here. What's the link between the two APIs?

Copy link
Member Author

@inancgumus inancgumus May 21, 2024

Choose a reason for hiding this comment

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

Explained here: #1594 (comment).

Base automatically changed from async/keyboard to main-browser-async May 22, 2024 10:20
down and up are closely related to move because without a move, users
will only be able to click (well, down or up) on the same part of the
virtual screen.
@inancgumus inancgumus merged commit a9959ae into main-browser-async May 22, 2024
2 checks passed
@inancgumus inancgumus deleted the async/mouse branch May 22, 2024 10:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Area: browser The browser module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants