-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
fix(Input): add handling of input's ref #1581
Conversation
cb0e4cf
to
2857c18
Compare
src/elements/Input/Input.js
Outdated
return createHTMLInput(child, { | ||
defaultProps: htmlInputProps, | ||
overrideProps: this.handleInputOverrides, | ||
}) |
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.
Use factory instead of cloneElement
Codecov Report
@@ Coverage Diff @@
## master #1581 +/- ##
==========================================
+ Coverage 99.74% 99.74% +<.01%
==========================================
Files 141 141
Lines 2381 2384 +3
==========================================
+ Hits 2375 2378 +3
Misses 6 6
Continue to review full report at Codecov.
|
const ref = sandbox.spy() | ||
const wrapper = mount(<Input><input ref={ref} /></Input>) | ||
|
||
// ref.should.have.been.calledOnce() |
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.
@levithomason I can't get this test working, can you take a look?
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.
Refs are not passed through props. React makes them undefined. We'll have to accept the ref on a different prop name, or, update factories to include refs in the overrides. We should probably use a different prop name, such as, inputRef
.
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 reverted back to cloneElement
and updated handling of ref
, also updated tests now everything works as expected. @levithomason thanks for the point to right direction
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.
Perfect, thanks!
Released in |
Fixes #1536.