Skip to content

feat: Add event modifiers #590

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

Merged
merged 5 commits into from
Jul 28, 2021

Conversation

nandi95
Copy link
Contributor

@nandi95 nandi95 commented May 11, 2021

linked #551

@nandi95
Copy link
Contributor Author

nandi95 commented May 11, 2021

@cexbrayat
Can we compile a list of what modifiers we need?

Asking because I don't get why in testing we would do something like click.prevent when preventing is the component's responsibility no?
Apart from that:

click.right|left|middle|ctrl
I imagine keyup|down.{keycode}

@cexbrayat
Copy link
Member

I think if we handle what's currently in the code base, that would be a nice start https://github.com/vuejs/vue-test-utils-next/blob/7a43f0756e72b3f10c98ebe0135347f5b53a5023/src/createDomEvent.ts#L7-L28

@lmiller1990
Copy link
Member

If we don't have any intention of doing this, I'd like to close it. Any update?

@nandi95
Copy link
Contributor Author

nandi95 commented Jul 8, 2021

I'll continue this on the coming weekend

@lmiller1990
Copy link
Member

No rush, just determining if it's ongoing work or not. Sounds like it is, happy to leave it open 👍

@nandi95 nandi95 marked this pull request as ready for review July 10, 2021 14:40
@@ -40,7 +18,7 @@ interface TriggerOptions {
}

interface EventParams {
eventType: DomEventName | string
eventType: DomEventName
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why was this also a string?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure 🤔

@nandi95 nandi95 requested a review from lmiller1990 July 20, 2021 14:48
@lmiller1990
Copy link
Member

Oh neat, it's green!

Anything specific you think is worth manually testing, or we good to merge?

@nandi95
Copy link
Contributor Author

nandi95 commented Jul 26, 2021

Oh neat, it's green!

Anything specific you think is worth manually testing, or we good to merge?

I don't think this cause any side effects so we good to merge.
I just tested it by checking the IDE makes the suggestions I expect.

Only thing could you resolve that comment about the interface EventParams?

@lmiller1990
Copy link
Member

Not sure on that EventParams type.

@lmiller1990 lmiller1990 merged commit 07481e0 into vuejs:master Jul 28, 2021
@lmiller1990
Copy link
Member

Will release sometime in the next few days, see if we can clean up a few other issues first. Thanks for this!!

@nandi95 nandi95 deleted the feature/add-event-modifiers branch July 28, 2021 05:27
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