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(type): support special charaters (like {enter}) #308

Merged
merged 4 commits into from Jun 4, 2020

Conversation

kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Jun 4, 2020

What: feat(type): support special characters (like {enter})

Why:

Closes #306
Closes #301
Closes #235
Closes #181
Closes #172
Closes #31

How:

Oh boy... Totally reworked the type functionality. Also made some pretty cool utilities for the tests which might be useful elsewhere.

Checklist:

  • Documentation
  • Tests
  • Typings
  • Ready to be merged

@codecov
Copy link

codecov bot commented Jun 4, 2020

Codecov Report

Merging #308 into master will increase coverage by 0.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   98.93%   99.20%   +0.27%     
==========================================
  Files           1        2       +1     
  Lines         188      253      +65     
  Branches       56       66      +10     
==========================================
+ Hits          186      251      +65     
  Misses          2        2              
Impacted Files Coverage Δ
src/index.js 98.75% <ø> (-0.19%) ⬇️
src/type.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c4746b...1cf8743. Read the comment docs.

@kentcdodds kentcdodds merged commit d38e493 into master Jun 4, 2020
@kentcdodds kentcdodds deleted the pr/drastically-improve-type branch June 4, 2020 23:29
@kentcdodds
Copy link
Member Author

🎉 This PR is included in version 11.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Collaborator

@malwilley malwilley left a comment

Choose a reason for hiding this comment

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

Wow, looks great! Really love what you've done to the tests. Sorry to add a review after you've merged, but I had a few thoughts that I thought I'd share (which I wrote before merging but I'm too slow! 😬)

})
}

if (currentElement().tagName === 'BUTTON') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good thinking here! I didn't even think about non-text elements. Now that I am though, would it be appropriate to also handle <input type="button" />, <input type="submit" />, etc?


await userEvent.type(input, '{esc}')

expect(getEventCalls()).toMatchInlineSnapshot(`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is such a slick method of testing. I ran into this same problem where tests became super hairy and difficult to reason about, but this is awesome 🤯


> **A note about modifiers:** Modifier keys (`{shift}`, `{ctrl}`, `{alt}`,
> `{meta}`) will activate their corresponding event modifiers for the duration
> of type command or until they are closed (via `{/shift}`, `{/ctrl}`, etc.).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do like the extra control we get here by adding {/shift} (compared to cypress), but worry that maybe it's a bit too verbose for most use cases.

Perhaps you could release modifier keys at the end of the type statement if no closing modifiers are provided? That could be the best of both worlds. I don't think there's any use case for keeping these keys held down at the end of a type statement (since you don't get the event modifiers after it's done).

...newEventOverrides,
})

return {eventOverrides: newEventOverrides}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you want to return both eventOverrides AND newEventOverrides here. Otherwise multiple held modifiers don't work correctly (such as userEvent.type('{ctrl}{meta}{enter}'))

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, we merge the old eventOverrides with the new ones elsewhere so we're good here 👍

@kentcdodds
Copy link
Member Author

@malwilley, love all the feedback. Would you like to make a pull request to implement it all?

@malwilley
Copy link
Collaborator

Sure thing!

@kentcdodds
Copy link
Member Author

We're moving user-event to within dom-testing-library and I just added the auto-release feature :) testing-library/dom-testing-library@bebfc74

If you'd like, there's plenty of opportunity to help still: testing-library/dom-testing-library#616

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants