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

"closeOnSelection" property(Multiple selection out of box) #319

Closed
serh11p opened this issue Jan 30, 2018 · 5 comments
Labels

Comments

@serh11p
Copy link

@serh11p serh11p commented Jan 30, 2018

I want to propose new Prop named closeOnSelection.

What happened, Problem description:
This codesandbox-example(Your Multi-select example with latest Downshift version) contains hardcoded solution: Menu doesn't close(as it were), it open after close. Because of this, the highlightedIndex is reset; as well as existence of a lot of handlers that just control the state of isOpen.

Suggested solution:

  defaultProps = {
    // ...
    closeOnSelection: true,
    // ...
  }
  
  selectItem = (item, otherStateToSet, cb) => {
    otherStateToSet = pickState(otherStateToSet)
    this.internalSetState(
      {
        ...this.props.closeOnSelection
          ? {
              isOpen: false,
              highlightedIndex: this.props.defaultHighlightedIndex, 
            }
          : {},
        selectedItem: item,
        inputValue:
          this.isControlledProp('selectedItem') &&
          this.props.breakingChanges.resetInputOnSelection
            ? this.props.defaultInputValue
            : this.props.itemToString(item),
        ...otherStateToSet,
      },
      (...args) => {
        this.focusInput()
        typeof cb === 'function' && cb(...args)
      },
    )
  }

!NOTE: At now this will work fine only for the case with Input element, because it's focusing after select.

@serh11p serh11p added the enhancement label Jan 30, 2018
@kentcdodds

This comment has been minimized.

Copy link
Member

@kentcdodds kentcdodds commented Jan 30, 2018

Seems like a good idea!

What if we made the solution more powerful. What if we added a prop called modifyStateChange and we put it here:

stateToSet = this.props.modifyStateChange(stateToSet, state)

Where the default would simply return stateToSet but you could provide your own function that could modify the state that will be set. You could check the type property of stateToSet against this list to know whether you want to modify how the state is changed.

This would actually allow people to not have to control state quite as much I think which would be pretty handy!

What do you think?

@serh11p

This comment has been minimized.

Copy link
Author

@serh11p serh11p commented Jan 30, 2018

@kentcdodds, Yeah! Looks great! i like this solution 👍, thanks.
And, the last important problem for me 😊 - its .focusInput(). I think, focusing on Input element after reset - is bad pattern. What if we focus last focused element BEFORE reset? This will be flex solution.
Probably need to Cash this element...🤔
What do you think?
Migrated #323

@kentcdodds

This comment has been minimized.

Copy link
Member

@kentcdodds kentcdodds commented Jan 30, 2018

What if we focus last focused element BEFORE reset?

Yep, I love that 👍

Would you like to work on both of those? They should be in different PRs I think :)

@serh11p

This comment has been minimized.

Copy link
Author

@serh11p serh11p commented Jan 30, 2018

Yes, i can. Of course, different.
I'll start a little bit later(in 2-3 hours).
Thanks for support!

@kentcdodds

This comment has been minimized.

Copy link
Member

@kentcdodds kentcdodds commented Jan 30, 2018

Loving the ideas coming here. Thank you!

@serh11p serh11p mentioned this issue Jan 30, 2018
0 of 3 tasks complete
@serh11p serh11p closed this Feb 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.