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
| } | ||
| stateToSet.type = stateToSet.type || Downshift.stateChangeTypes.unknown | ||
|
|
||
| if (stateToSet.hasOwnProperty('inputValue')) { |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
Yes, it does. Thanks for clearing things up! :)
robin-drexler
force-pushed
the
fix-217-input-cursor
branch
from
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
robin-drexler
force-pushed
the
fix-217-input-cursor
branch
from
0e5eb83 to
c8251e4
Compare
|
Implemented the function version of Rebased and force-pushed to remove the story. |
kentcdodds
left a comment
kentcdodds
left a comment
There was a problem hiding this comment.
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!
robin-drexler
force-pushed
the
fix-217-input-cursor
branch
from
cab0a96 to
54adb16
Compare
|
Would you like to add yourself to the contributors table? |
|
Thank you so much for your help. :) |
3 participants
#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?