Issue 157: trigger onConfirm after set state

[samtsai]

onConfirm is currently triggered before setState is called,
to keep proper state with what is sent to onConfirm, setState
should be called then onConfirm.

This is to resolve #157

[samtsai]

Have some linting errors, let me fix

[samtsai]

@tvararu I wasn't quite sure how the selected property was being used so I opted to just send newQuery. During testing, selected was -1 so option[selected] was not returning what I expected.

[tvararu]

I opted to just send newQuery

👍 That's fine. options[selected] was weird given that newQuery exists in the same scope.

The second argument to setState should be a callback function. You're passing the result of the onConfirm() invocation. So your code (when this.props.confirmOnBlur === true) at runtime is:

this.setState({
  focused: null,
  menuOpen: newState.menuOpen || false,
  query: newQuery,
  selected: null
}, undefined)

With the added side effect of running onConfirm in order to produce the undefined argument for setState, which shouldn't actually solve the underlying issue (if it did, that's curious).

Instead you want this (added () =>):

this.setState({
  focused: null,
  menuOpen: newState.menuOpen || false,
  query: newQuery,
  selected: null
}, () => this.props.confirmOnBlur ? this.props.onConfirm(newQuery) : null)

Also apply the same change to the other place where this is used. :)

[tvararu]

options[selected] was weird given that newQuery exists in the same scope.

Actually, disregard what I said. It should be options[selected]. options[-1] returning undefined is correct behaviour. In which tests did you find this to be unexpected?

[samtsai]

Thanks @tvararu, I will correct the callback application, but as far as sending options[selected] to onConfirm, wouldn't that be misleading? I thought onConfirm is meant to receive the selected query, in which case options[-1] was giving undefined.

What I mean is, in normal usage the selected property was not being updated in the state in all cases or would come back as -1. I was just using any of the example Autocompletes.

[tvararu]

@samtsai it's meant to receive the selected option which can be an object. query is just a printable string that gets inlined as part of the input. For most usecases these two concepts are identical, but the autocomplete does support more advanced use cases using the templates.inputValue option.

[samtsai]

Ok makes sense, I'll need to take a look at this a little longer, going to be on vacation rest of this weekend, so I'll pick it up next week.

Likely, I'll want to update my tests to reflect your comments.

[tvararu]

I opted to just send newQuery

👍 That's fine. options[selected] was weird given that newQuery exists in the same scope.

The second argument to setState should be a callback function. You're passing the result of the onConfirm() invocation. So your code (when this.props.confirmOnBlur === true) at runtime is:

this.setState({
  focused: null,
  menuOpen: newState.menuOpen || false,
  query: newQuery,
  selected: null
}, undefined)

With the added side effect of running onConfirm in order to produce the undefined argument for setState, which shouldn't actually solve the underlying issue (if it did, that's curious).

Instead you want this (added () =>):

this.setState({
  focused: null,
  menuOpen: newState.menuOpen || false,
  query: newQuery,
  selected: null
}, () => this.props.confirmOnBlur ? this.props.onConfirm(newQuery) : null)

Also apply the same change to the other place where this is used. :)

[tvararu]

See comment, looks good otherwise. 👍

[tvararu]

Few changes, thanks for coming back to this. 👍

Looks much cleaner!

[tvararu]

This should be:

setStateCallback = () => this.props.onConfirm(options[selected]) 

[tvararu]

This should be:

}, () => this.props.onConfirm(selectedOption))

[NickColley]

Should this be documented somewhere?

Edit: Actually, I've reread it this does not introduce anything new. 👍

[tedw]

Any update on this? Thanks!

[colinrotherham]

I've rebased this one with main to help us review