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

handleStateChange confusion around controlling isOpen #206

Closed
jole78 opened this issue Sep 29, 2017 · 14 comments · Fixed by #208
Closed

handleStateChange confusion around controlling isOpen #206

jole78 opened this issue Sep 29, 2017 · 14 comments · Fixed by #208

Comments

@jole78
Copy link

jole78 commented Sep 29, 2017

Problem description:
I'm trying to create a dropdown that contains a couple of choices that the user can select (a checkbox list one could say). It's basically a multiple select thingy.

So I wan't to keep the dropdown menu open while the user is interacting with it.
So, I know that I then have to to take "control" over the isOpen prop and supply that to downshift. That all works fine.

The issue that I'm facing is that if the user clicks outside the downshift component (the root component) the menu should close.

I found this piece of code in the example:

  handleStateChange = changes => {
    const {isOpen, type} = changes
    if (type === Downshift.stateChangeTypes.mouseUp) {
      this.setState({isOpen})
    }
  }

That seems to do the trick...but...for one it seems a little bit "complicated" (or how should I put it) and I really don't know why I need this and how it all works together.

Suggested solution:
Either better documentation about this type of situation or some other means to tell downshift that when the user clicks outside just close it, ignoring isOpen prop.
React Bootstrap has a prop called rootClose on its overlays that handles this for example.

@kentcdodds
Copy link
Member

Thanks for the issue @jole78, I knew we'd have to deal with this eventually.

Here's how things work... There are 4 parts of state in downshift: isOpen, highlightedIndex, selectedItem, and inputValue. We have an API that allows you to control the internal state of the component. When you're controlling any part of the state, then we need to only call setState for the parts of the state that downshift is controlling and we call onStateChange with anything that changed (whether or not it's controlled).

As noted above, when you control any state, we don't call setState internally and instead you're responsible for updating the state. In fact, we can't control any state that you're controlling or we'll get into really confusing situations. This is consistent with controlled form fields. Due to this fact, you need to make sure you update your own version of the state based on whatever criteria you determine. This is why onStateChange exists. It's suggestions to you. You can use the information to update your own state or not.

There are a lot of reasons state can change and it's not easily determinable without the type property on the changes object. The isOpen state in particular has at least 5 ways to change (probably more, I didn't actually check). So that's why the stateChangeTypes exists.

It's undocumented because I didn't love the API and most folks can get a long way without needing to know it exists. But I think that things have been around long enough to say that no better API will materialize and we can set this one in stone.

React Bootstrap has a prop called rootClose on its overlays that handles this for example.

Do they allow you to control the isOpen state like downshift does? I truthfully feel like this is a bit complicated because it's so powerful.

I'm thinking the solution is to simply document the stateChangeTypes and make it an official part of the API.

Happy to discuss alternative solutions.

@jole78
Copy link
Author

jole78 commented Sep 29, 2017

@kentcdodds thx for the reply. It’s Friday night in Sweden so I’ll have to think this over and get back to you next week. Have a great weekend Kent.

@jole78
Copy link
Author

jole78 commented Oct 2, 2017

@kentcdodds if I'm not misreading how React Bootstrap uses this they allow you to set rootClose on an overlay component and if you do you must specify an onHide event handler (that becomes a required prop). That usually involves you controlling the "show" prop on the overlay (via your state)...so yes, they allow you to control the isOpen state of the overlay. ref (https://react-bootstrap.github.io/components.html#overlays).

Is that perhaps a solution?? Something like you can set a prop on downshift (like rootClose or something) and then downshift will call your onHide event handler when that occurs withing downshift.

Truthfully I feel this code below forces me to know a little too much about the inner workings of downshift, as you also clearly agreed with...right?

    handleStateChange = changes => {
        const { isOpen, type } = changes;
        if (type === DownShift.stateChangeTypes.mouseUp) {
            this.isOpen = isOpen;
        }
    };

Another solution might be another stateChangeType which more clearly would indicate that the user clicked outside of the downshift component?

@kentcdodds
Copy link
Member

I can understand your concern. One thing that I want to avoid is adding a bunch of props to do things which can be done with what's already available.

However, I think this is a pretty common use case, so I'm willing to add a prop for an onOuterClick event handler which we would call here. The docs for onOuterClick would indicate that this is just a helper prop and the same could be accomplished using onStateChange.

I think the implementation should be quite simple. Just in time for hacktoberfest 😉

Steps to solve:

  1. Document the new prop in the README.
  2. Test the new prop's functionality (probably easiest to just do it in downshift.misc.js).
  3. Implement the new functionality by adding a prop type around here and calling it in the callback here (this.reset({type: Downshift.stateChangeTypes.mouseUp}, this.props.onOuterClick))

Please make sure to follow the CONTRIBUTING.md.

Thanks!

@kentcdodds
Copy link
Member

Note, first person to "claim" this issue by saying "I'm working on this right now" will have their PR iterated on and merged :)

@divyenduz
Copy link
Collaborator

I'm working on this right now

@jole78
Copy link
Author

jole78 commented Oct 2, 2017

Looks fair enough. Saw that react-select has something similar called onClickOutside or something like that so it seems reasonable. Thx

@Joroze
Copy link

Joroze commented Oct 3, 2017

Hey everyone, I wanted to report a possible bug (unless I misread the documentation), and I don't mean to steal @jole78's spotlight, but I think this issue is related to what I'm talking about.

I've been playing around with Downshift and I realized that there may be a bug related to how onStateChange provides the wrong change.type value if you have more than one Downshift component being rendered at the same time.

For example... I copied the provided 'Multiple Select' sample, and just made a modification by rendering an extra Downshift dropdown component below the first one.

With that said, when you click an item in the dropdown list, the changeType from onStateChange is now mouseUp ("__autocomplete_mouseup__"), when it should be something else (probably "__autocomplete_controlled_prop_updated_selected_item__").

Since it provides mouseUp here, the changes's isOpen property value is false, and the dropdown state will be set to close.

Try it out for yourself here. https://codesandbox.io/s/8pywk9vo8

Any ideas how to resolve this?

  handleStateChange = changes => {
    const {isOpen, type} = changes
    if (type === Downshift.stateChangeTypes.mouseUp) {
      this.setState({isOpen})
    }
  }

@kentcdodds
Copy link
Member

Yeah, that's a really odd thing you're doing there... I don't have the time to explain what's going on (someone else can take a crack at it if you want), but if you explain what you're trying to accomplish then maybe I can help you know how to do it properly...

@Joroze
Copy link

Joroze commented Oct 4, 2017

Hi @kentcdodds. First of all, I'm assuming this upcoming onOuterClick functionality will resolve what I'm talking about, correct?

Anyways, I'm trying to accomplish rendering a Downshift dropdown component in each accordion panel's content of a Semantic-UI-React accordion. https://react.semantic-ui.com/modules/accordion#accordion-example-shorthand

Everything worked fine, until I wanted to essentially close a Downshift dropdown on blur, using this implementation:

  handleStateChange = changes => {
    const {isOpen, type} = changes
    if (type === Downshift.stateChangeTypes.mouseUp) {
      this.setState({isOpen})
    }
  }

This actually worked too for the first accordion panel. But when I started to add more than one accordion panels dynamically (with the Downshift dropdown component in the content of each accordion panel), then the bug occurs. I realized regardless of the accordion panels being open/closed, the content is always rendered but hidden in some way.

With that said I experimented further by just rendering two Downshift dropdown components next to each other and I also replicated the bug.

Earlier today, I was able to work around a fix for my situation by just rendering one of the Downshift dropdown components at a time based on the current active accordion. This way, no more than one Downshift component can be rendered at a time, working around the issue.

With all of that said, that's my story and how I came across this. :)

kentcdodds pushed a commit that referenced this issue Oct 4, 2017
This PR is not ready to be merged yet. When merged, this will fix #206

This PR does the following (Will appreciate early feedback and comments):-

1. Add onOuterClick description to README. I can remove the link to a specific issue from there

2. Add onOuterClick to propTypes with type PropTypes.func

3. Call this.props.onOuterClick as a callback to reset function when it is called under onMouseUp event

4. Add a test case for newly added functionality. This is still incomplete. I started with the test case expecting it to fail in its current state where I open the menu and simulate a outside click with click on document.body. With the current state of the onOuterClick callback this should have resulted in isOpen to be false there by failing the test but it is not the case. Exploring.

Also, to simulate the click on document.body. I borrowed the function `mouseDownAndUp` from `downshift.lifecycle.js`. I can either move this test over there or expose that function in a better way than duplicating.

Let me know your feedback on the approach? Also, please point out if you believe that I understood the problem incorrectly.
@kentcdodds
Copy link
Member

Ah, the problem is that you're sharing the isOpen state between the two. So if the first one collapses because I clicked on the second one, then they'll both collapse and that will happen before the second has a chance to select the item.

You can solve this by keeping the isOpen state separate. I suggest creating another component that's responsible for this state and renders the Downshift component, then you can render two versions of that component.

In any case, onOuterClick has been merged and released 🎉 Thanks everyone!

@Joroze
Copy link

Joroze commented Oct 4, 2017

@kentcdodds ah, duh... Thanks! 👍

@chrisjlee
Copy link

chrisjlee commented Nov 27, 2017

@kentcdodds Found this issue to be useful; especially your rational for using handleStateChange.

So is it certain that the Downshift.stateChangeTypes will be an official part of the API? Or do you have an issue to gather consensus?

If so would like to PR to update the docs for these usages.

@kentcdodds
Copy link
Member

Yeah, I think we'll make the stateChangeTypes official. Anyone can feel free to make a pull request to document it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants