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

WIP: add setNativeValue for change event #153

Closed
wants to merge 1 commit into from

Conversation

kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Aug 4, 2018

What: This overrides the fireEvent.change function to one that allows users to set the target.value using a special setNativeValue function.

Why: (#152) Because React tracks when you set the value property on an input to keep track of the node's value. When you dispatch a change event, it checks it's last value against the current value and if they're the same it does not call any event handlers (as no change has taken place as far as react is concerned). So we have to set the value in a way that React's value setter function will not be called, which is where the setNativeValue comes into play.

How: Development of the setNativeValue function was a team effort: facebook/react#10135 (comment)

This is kinda cheating because it's overriding fireEvent.change. I think I'd rather we do this within dom-testing-library directly.

I'm also not super happy with the API:

fireEvent.change(input, {target: {value: 'hi'}})

It makes sense to me because in your onChange handler if you want the value you'll get it from event.target.value, but I worry that this API will make people think they can change other things about the target as well... I suppose we could make that work by Object.assigning all target properties onto the given node (while still treating the value property specially) 🤔

Another thing I've considered is just accepting a value option:

fireEvent.change(input, {value: 'hi'})

But I don't like that because the 2nd argument is supposed to be event init options... I'm open to feedback here.

Also, this PR is incomplete because we still have to handle select, mouseEnter, and mouseLeave. I'm not sure how to handle those. More investigation is needed.

I'm thinking that we should make these changes in dom-testing-library instead of here and we can address each of these one-by-one.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table N/A

@codecov
Copy link

codecov bot commented Aug 4, 2018

Codecov Report

Merging #153 into master will decrease coverage by 9.37%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #153      +/-   ##
==========================================
- Coverage     100%   90.62%   -9.38%     
==========================================
  Files           1        1              
  Lines          19       32      +13     
  Branches        3        8       +5     
==========================================
+ Hits           19       29      +10     
- Misses          0        2       +2     
- Partials        0        1       +1
Impacted Files Coverage Δ
src/index.js 90.62% <76.92%> (-9.38%) ⬇️

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 9667967...799d6ca. Read the comment docs.

@kentcdodds
Copy link
Member Author

This is superseded by testing-library/dom-testing-library#85

@kentcdodds kentcdodds closed this Aug 4, 2018
julienw pushed a commit to julienw/react-testing-library that referenced this pull request Dec 20, 2018
<!--
Thanks for your interest in the project. Bugs filed and PRs submitted are appreciated!

Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Also, please make sure you're familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

If you're new to contributing to open source projects, you might find this free
video course helpful: http://kcd.im/pull-request

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->

**What**: Vague documentation (issue testing-library#153 )

<!-- Why are these changes necessary? -->

**Why**: To clarify usage to users

<!-- How were these changes implemented? -->

**How**: Added documentation

<!-- Have you done all of these things?  -->

**Checklist**:

<!-- add "N/A" to the end of each line that's irrelevant to your changes -->

<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->

- [x] Documentation N/A
- [ ] Tests N/A
- [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [ ] Added myself to contributors table <!-- this is optional, see the contributing guidelines for instructions -->

<!-- feel free to add additional comments -->
@kentcdodds kentcdodds deleted the pr/remove-simulate branch April 5, 2019 13:37
lucbpz pushed a commit to lucbpz/react-testing-library that referenced this pull request Jul 26, 2020
…ests

test: 💍 add Vue tests for type, dblclick, selectoptions
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.

None yet

1 participant