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

fix: cursor jumping to end of input #229

Closed

Conversation

robin-drexler
Copy link
Contributor

@robin-drexler robin-drexler commented Oct 17, 2017

#217

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:

  • Documentation
  • Tests
  • Ready to be merged -> docs missing
  • Added myself to contributors table

@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?

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

downshift-js#217
@codecov-io
Copy link

Codecov Report

Merging #229 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #229   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         271    273    +2     
  Branches       63     64    +1     
=====================================
+ Hits          271    273    +2
Impacted Files Coverage Δ
src/downshift.js 100% <100%> (ø) ⬆️

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 1385fc9...4347e80. Read the comment docs.

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.

Things are looking pretty good. Just an idea.

@@ -268,6 +270,14 @@ class Downshift extends Component {
internalSetState(stateToSet, cb) {
let onChangeArg
const onStateChangeArg = {}

if (stateToSet.hasOwnProperty('inputValue')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better be careful here. stateToSet can be a function. Also, this.getStateAndHelpers() will get us the old state, not any of the new state. So let's move this down to right before return nextState and then we can do:

this.props.onInputValueChange(
  stateToSet.inputValue,
  {...this.getStateAndHelpers(), ...nextState},
)

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this cause inputValue being read from stale props again?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, because nextState will have the inputValue.

But actually, it should probably be this instead:

this.props.onInputValueChange(
  stateToSet.inputValue,
  {...this.getStateAndHelpers(), ...stateToSet},
)

And we could put it right below stateToSet.type = ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried that here, but placing the onInputValueChange inside the setState function, causes the children to re-render with the old value once, still leading to the cursor jumping to the end.

@robin-drexler
Copy link
Contributor Author

Sorry, I need to close this PR. github is not updating new commits in here. I can't figure out why. Will open a new one.

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.

Cursor jumps to end of input when inputValue is a controlled prop
3 participants