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
Bugix/fire type events on active element #299
Bugix/fire type events on active element #299
Conversation
Codecov Report
@@ Coverage Diff @@
## master #299 +/- ##
==========================================
+ Coverage 98.91% 98.93% +0.01%
==========================================
Files 1 1
Lines 185 188 +3
Branches 55 56 +1
==========================================
+ Hits 183 186 +3
Misses 2 2
Continue to review full report at Codecov.
|
I can see that my Code Coverage report has failed and is a blocking issue, but I'm not quite sure how to tackle it. I would appreciate any advice / suggestions 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking super. Just a few suggestions.
We should probably spend a bit of time improving code coverage in this project in general, but I think your stuff is pretty well tested so I wouldn't worry about the bot :) Thanks!
src/index.js
Outdated
key, | ||
keyCode, | ||
charCode: keyCode, | ||
}) | ||
|
||
const isTextPastThreshold = | ||
(actuallyTyped + key).length > (previousText + computedText).length | ||
(actuallyTyped() + key).length > | ||
(currentElement().maxLength || text.length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what I'd prefer is to turn computedText
into a function called computeText
which would be used in the sync case as well as here. I think it'd be something like:
const computeText = () =>
currentElement().maxLength > 0
? text.slice(0, Math.max(currentElement().maxLength - actuallyTyped().length, 0))
: text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this to the way I think it should work, but I think I may have misunderstood.
The way I understand this is....
Previously we could calculate how many total characters we can type based on the value that was already in the input before we started using previousText
. We could then use this as a check against what we've actuallyTyped
.
Now we are using the current value (actuallyTyped
) of the element (because the previousValue
may no longer apply to the current element), so we're computing how many more characters we can add at any point (not total).
I had originally assumed the isTextPastThreshold
check would be:
const isTextPastThreshold =
(currentValue() + key).length >
(currentValue() + computeText()).length
But this felt wrong, because we're checking how many more characters we can add and then adding it to the current value and that doesn't necessarily mean it's the correct length.
So, I think what I've done is correct, but let me know if I've completely missed the point 😄
(Also, to note, where I'm saying actuallyTyped
I've renamed it to currentValue
in the code because this felt more reflective of the actual usage now)
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking super. Just two quick suggestions and then I think we should be good to go!
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic. Thank you!
@all-contributors please add @kayleighridd for bugs, code, and tests |
I've put up a pull request to add @kayleighridd! 🎉 |
🎉 This PR is included in version 11.0.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
🎉 🙌 Thanks! |
Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the |
FYI this broke some of my tests coming from |
What:
I raised an issue (#295) to allow focus to change between type events. This supports scenarios where focus is switched to a second input in between
keyDown
andkeyUp
events, for example.This PR also contains a lot of formatting fixes that happened automatically on commit.
Why:
I personally, needed this to test a multiple input field, but this also more closely matches user behaviour.
How:
I've updated the
type
function to:element
provideddocument.activeElement
when firing individual eventsdocument.activeElement
's value andmaxLength
when checkingisTextPastThreshold
so that we check the relevant element's threshold before firinginput
computedText
andpreviousText
variables inside theallAtOnce
check as it's no longer needed elsewhere.-
allAtOnce
functionality is unchanged.(As there are a lot of formatting changes to sift through, the functionality changes described above can be found around this comment: https://github.com/testing-library/user-event/pull/299/files#r431147168)
Checklist:
The work in this PR looks like it conflicts with the work / relates to the conversations happening in #231
So, this may be blocked until that PR is resolved.