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: add capslock to userEvent.type #507

Merged
merged 6 commits into from Nov 30, 2020

Conversation

Sonic12040
Copy link
Collaborator

What: Adding CapsLock to the modifier key support so that tests using CapsLock can be implemented.

Why: When typing and using the CapsLock modifier, we need to be able to write tests that implement the CapsLock key.

How: Added a test to the type-modifiers file, then implemented the suggested changes from Kent C Dodds office hours to the type file.

Checklist:

  • Documentation
  • Tests
  • [N/A] Typings
  • Ready to be merged

@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #507 (9cfbb8f) into master (f7620ab) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #507   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          615       625   +10     
  Branches       200       200           
=========================================
+ Hits           615       625   +10     
Impacted Files Coverage Δ
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 f7620ab...f978e9e. Read the comment docs.

input[value=""] - pointerup
input[value=""] - mouseup: Left (0)
input[value=""] - click: Left (0)
input[value=""] - keydown: CapsLock (20)
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this one is tricky. Basically for caps lock, it should keydown and keyup here, but preserve the modifier state. Hmmm...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, do you think a PR will also be needed to fireEvent for the case of CapsLock?

Copy link
Member

Choose a reason for hiding this comment

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

No, fireEvent is all about dispatching individual events in complete isolation. It is not concerned about state. This will only need to happen here in user-event. It's possible we won't be able to re-use the existing createModifierCallbackEntries for this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Scratch that. Updated with a switch/case. I'm not sure if it's initially beneficial to write it that way, but I didn't know if other modifier keys might also require the behavior at some point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Saw the comment after. I can write a separate function for it if preferred?

Copy link
Member

Choose a reason for hiding this comment

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

@Sonic12040, could you try removing the custom logic and inlining it for the caps lock. I think that will result in simpler code in this case. I'm worried that this is a hasty abstraction (AHA Programming) and I think the small amount of duplication will be better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated with a duplicate function that performs the CapsLock behavior.

input[value="a"] - input
"{CURSOR}" -> "a{CURSOR}"
input[value="a"] - keyup: a (97)
input[value="a"] - keyup: CapsLock (20)
Copy link
Member

Choose a reason for hiding this comment

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

And then this should keydown and keyup and disable the modifier state. I'm afraid this one isn't quite as simple as I thought at first.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Thanks for your patience on this! I think we're almost there.

src/type.js Outdated
@@ -42,6 +42,12 @@ const modifierCallbackMap = {
keyCode: 93,
modifierProperty: 'metaKey',
}),
...createCapsLockModifierCallbackEntries({
Copy link
Member

Choose a reason for hiding this comment

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

Rather than using a function, maybe it would make more sense to just inline this one with a comment explaining why it can't use createModifierCallbackEntries?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Sorry about the delay; I missed this comment at first.

README.md Outdated Show resolved Hide resolved
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

I love it. Thank you for taking the time to make this happen!

@kentcdodds kentcdodds merged commit 9344db6 into testing-library:master Nov 30, 2020
@kentcdodds
Copy link
Member

@all-contributors please add @Sonic12040 for code, tests, and docs

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @Sonic12040! 🎉

@kentcdodds
Copy link
Member

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

@github-actions
Copy link

🎉 This PR is included in version 12.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants