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 cursor jumping to end of input #230
Fix cursor jumping to end of input #230
Conversation
I've added a story (to be removed again) to showcase the issue and to tinker around with. |
Codecov Report
@@ Coverage Diff @@
## master #230 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 4 4
Lines 277 283 +6
Branches 63 66 +3
=====================================
+ Hits 277 283 +6
Continue to review full report at Codecov.
|
src/downshift.js
Outdated
@@ -287,6 +290,14 @@ class Downshift extends Component { | |||
onChangeArg = stateToSet.selectedItem | |||
} | |||
stateToSet.type = stateToSet.type || Downshift.stateChangeTypes.unknown | |||
|
|||
if (stateToSet.hasOwnProperty('inputValue')) { |
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.
Unfortunately, this wont work with when we call reset
which users can do themselves or when the user clicks outside the menu... 🤔 This is because stateToSet
in that case is a function, but it does set the inputValue
. So I'm thinking that we can do this, but we also have to handle the case where stateToSet
is a function. I think the best thing to do would be to test this by calling reset
and make sure that onInputValueChange
is called in that case (in addition to the other tests you already have). Then we'll figure out the best way to implement that.
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.
Did you see my comment here? #229 (comment)
I'm not sure yet that invoking the onInputValueChange
callback inside the setState
function works at all to fix the issue at hand. I'd like to prove that first before addressing issues like the different incarnations of stateToSet
.
Sorry for the confusion with two opened PRs. :/
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 did see your comment. But as I said, the existing code will never call onInputValueChange
when reset
is called. We definitely need to call onInputValueChange
any time the inputValue
is called.
I think that the solution is good as is, then we add more code for the case when the stateToSet
is a function (which only applies in the reset
case). Luckily for us, when reset
happens, the original issue won't be a problem.
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.
The issue I'm having is that calling onInputValueChange
inside setState
function does not appear to fix the issue with the cursor jumping to the end of the input.
Or am I misunderstanding and with "the existing code" you were referring to my original version, where the onInputValueChange
call was placed above the setState
?
Sorry, I'm a bit confused right now.
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.
Ok, so the code you have already is great. We're not going to change it.
The problem is that the code you have doesn't cover the reset
case because stateToSet
is a function in that case (and it has to be that way). So if we merged this PR as is it would work great for everything except for when the user clicks outside the menu or tabs out of the menu (which is when reset
is called).
To deal with this issue, we need more code. We need to handle the case where stateToSet
is a function. If it is, then we need to do that inside of setState
.
Luckily, when stateToSet
is a function, then we wont have the problem of the cursor jumping to the end of the input because the focus will be elsewhere anyway.
Does that make sense?
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.
Yes, it does. Thanks for clearing things up! :)
c7d998f
to
0e5eb83
Compare
provide an onInputValueChange callback to allow to update controlled prop before its old value is used to re-render the children causing the cursor in the input to jump to the end in the next render
0e5eb83
to
c8251e4
Compare
Implemented the function version of Rebased and force-pushed to remove the story. |
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.
This is looking perfect! Just need to add docs for the onInputValueChange
prop. If you could add that like the other props that'd be awesome. Thanks!
cab0a96
to
54adb16
Compare
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.
Super! Thanks!
Would you like to add yourself to the contributors table? |
Thank you so much for your help. :) |
#217
Continuation of #229
What:
Provide an onInputValueChange callback to allow to update controlled prop before its old value is used to re-render the children, causing the cursor in the input to jump to the end in the next render.
Why:
fixes #217
Checklist:
@kentcdodds I'd like to get your opinion on this change before adding documentation for it.
Do you think this is a direction worth pursuing?