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

Bugix/fire type events on active element #299

Conversation

kayleighridd
Copy link
Contributor

@kayleighridd kayleighridd commented May 27, 2020

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 and keyUp 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:

  • Set focus on the element provided
  • Reference the current document.activeElement when firing individual events
  • Reference the current document.activeElement's value and maxLength when checking isTextPastThreshold so that we check the relevant element's threshold before firing input
  • Moved the computedText and previousText variables inside the allAtOnce 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:

  • Documentation - N/A (I don't think this is required, but happy to add some if needed)
  • Tests
  • Typings - N/A
  • Ready to be merged

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.

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #299 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
src/index.js 98.93% <100.00%> (+0.01%) ⬆️

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 9fe76b4...34ddb49. Read the comment docs.

@kayleighridd
Copy link
Contributor Author

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 😄

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.

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 Show resolved Hide resolved
src/index.js Outdated
key,
keyCode,
charCode: keyCode,
})

const isTextPastThreshold =
(actuallyTyped + key).length > (previousText + computedText).length
(actuallyTyped() + key).length >
(currentElement().maxLength || text.length)
Copy link
Member

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

Copy link
Contributor Author

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)

src/index.js 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.

Looking super. Just two quick suggestions and then I think we should be good to go!

src/index.js Outdated Show resolved Hide resolved
src/__tests__/type.js Outdated Show resolved Hide resolved
kayleighridd and others added 2 commits June 4, 2020 16:06
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
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.

Fantastic. Thank you!

@kentcdodds kentcdodds merged commit a6f1326 into testing-library:master Jun 4, 2020
@kentcdodds
Copy link
Member

@all-contributors please add @kayleighridd for bugs, code, and tests

@allcontributors
Copy link
Contributor

@kentcdodds

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

@kentcdodds
Copy link
Member

🎉 This PR is included in version 11.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kayleighridd
Copy link
Contributor Author

🎉 🙌 Thanks!

@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 :)

@ScottG489
Copy link

FYI this broke some of my tests coming from 11.0.0 since I believe this is the only change in 11.0.1. I know this is an old change, but it was a little frustrating to see a patch version change break tests. I just wanted to call it out for visibility because I've used testing-library for a while and it's a quality project, so I'm sure you'd like to know.

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

3 participants