Downshift: Respect defaultInputValue when provided

[EnoahNetzach]

Hi, thanks a lot for this UI-less solution: it's been really easy to integrate with our existing UI lib as a substitution of a (really) old jQuery based one!

I'm having one single problem: I provide both a selectedItem and a defaultInputValue to downshift, and since inputValue and selectedItem are decoupled, I'd expect that in my initial rendering I would see defaultInputValue.

In practice this snippet of code in the constructor:

if (state.selectedItem != null) {
  state.inputValue = this.props.itemToString(state.selectedItem)
}

prevents that, effectively coupling the two fields.

I'd suggest to also check wether this.props.defaultInputValue is null in that if condition.

[kentcdodds]

Hmmm... But the only reason selectedItem would not be null is if you're specifying a defaultSelectedItem prop. The inputValue should reflect the selectedItem when the user hasn't made changes to the input value which is why that logic is the way it is. Can you show an example of the kind of UI experience you're trying to build so I can understand better why you want things to not work as they are?

[EnoahNetzach]

I'm talking about the initial render only (where the default values make sense), I'll show you what I mean shortly.

[EnoahNetzach]

Here a quick example.

I would like to maintain the decoupling you have when you're changing the input, but the selected item doesn't change yet:

even in the first render (here implemented by a pristine value in the state:

[EnoahNetzach]

The user experience I'd like to give is: since users haven't interacted with the dropdown yet, I'd like to show them all the viable options.

Now the experience they have is: they see an input without options, and they have to actively clear the input before options are shown

Example: here (I just took one official example, and added a defaultSelectedItem).

[kentcdodds]

That's very interesting. I'm inclined to agree with you. I'm thinking what might be better is an initialInputValue prop, or perhaps a more flexible prop that could allow you to specify the initial state of the entire component:

const ui = <Downshift initialState={{isOpen: true, inputValue: '', selectedItem: items[3], highlightedIndex: 3}} />

What would you say to that?

[EnoahNetzach]

My understanding was that it was exactly the role of the default_ props (defaultSelectedItem, defaultHighlightedIndex, defaultInputValue, defaultIsOpen), if not I completely missed their point..

Anyway, any solution looks equally promising (where the more flexible one could be useful moar broadly).

[kentcdodds]

That's part of the use case, but another part of the use case is what to do when downshift is reset. The distinction is slight and normally not noticeable. It's probably kinda confusing. But the reason the default isn't being picked up in this case is because the default doesn't make sense when there's a selected item. Default is interpreted to mean: "When there's no other value that makes sense." In this case there is a value that makes sense. And that value is the itemToString value of the default selected item. 😅

Sorry it's confusing. I think I'd merge a PR to add support for initialState. We'd definitely want tests and docs for it. I think it should be considered an advanced prop (for docs purposes).

[EnoahNetzach]

I got the distinction. I'll try to get a PR with the initial_ props ASAP.
I'd go down the distinct props, as opposed to the initialState, just to keep the same API.

I'll try also to add a short paragraph in the docs to explain the distinction.

Would that be OK?

[kentcdodds]

That sounds fine. Maybe we could get away with not having docs dedicated to each prop? I want to avoid confusion. Most people probably wont need these props so having a single section for "initial props" would probably be sufficient.

[EnoahNetzach]

agreed!

[EnoahNetzach]

I have a question: right now on reset only defaultHighlightedIndex is respected, but from your previous comment I thought the default props were meant to be used on reset.

This is the piece of code I'm talking about:

  reset = (otherStateToSet = {}, cb) => {
    otherStateToSet = pickState(otherStateToSet)
    this.internalSetState(
      ({selectedItem}) => ({
        isOpen: false,
        highlightedIndex: this.props.defaultHighlightedIndex,
        inputValue: this.props.itemToString(selectedItem),
        ...otherStateToSet,
      }),
      cb,
    )
  }

Shouldn't it be something more like:

  reset = (otherStateToSet = {}, cb) => {
    otherStateToSet = pickState(otherStateToSet)
    this.internalSetState(
      (state) => {
        return {
          isOpen: this.props.defaultIsOpen,
          highlightedIndex: this.props.defaultHighlightedIndex,
          inputValue: this.props.defaultInputValue || this.props.itemToString(this.props.defaultSelectedItem),
          selectedItem: this.props.defaultSelectedItem,
          ...otherStateToSet,
        }
      },
      cb,
    )
  }

where the current state is completely discarded in favour to the default props?

[kentcdodds]

Yeah, you're right. It really should be using the defaults in the reset. That's a design flaw. I sure wish that we'd realized this before I released 2.0.0 last week 😅

I think this is worth releasing 3.0.0 though. So let's change this and push it as a breaking change. Could you do that?

[kentcdodds]

I'm working on this at the moment and I've got a solution that I like.

The one difference from what we discussed is the changes to the reset function. I will document this, but the expectation of the reset function isn't to totally revert the state of downshift to the initial state, but instead to reset it to the last selected item. Based on how it's used internally and externally, that's the best expectation.

Practically what this means is that the selectedItem will remain the same, and the inputValue will be derived from that value. The highlightedIndex and isOpen state will be based on the defaultValue though.

If someone wanted to completely reset downshift, they could do so by passing overrides:

reset({selectedItem: null, inputValue: ''})
// or, fun fact, we expose setState too, though this is undocumented:
// setState({isOpen: false, highlightedIndex: null, selectedItem: null, inputValue: ''})

All that said, perhaps reset isn't the right name for this function. If you have a better name for it let me know. Thanks!

[kentcdodds]

:tada: This issue has been resolved in version 3.0.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: