Fix menu closing when click outside the menu box

[ytiurin]

This refers to the issue #2239

[coveralls]

Coverage increased (+0.3%) to 95.225% when pulling 25dc020 on ytiurin:master into 82910d9 on JedWatson:master.

[vitorhorta]

Is this PR merged? Looking forward for this fix!

[lifejuggler]

I was wondering when this will be merged in?

[eugensunic]

It has been merged. Just put the latest version in your package json On Feb 22, 2018 17:17, "Tom Choi" <notifications@github.com> wrote: I was wondering when this will be merged in? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2314 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AK8OH4BgtQ_D9OYGt8eDO2A3JP2wefgyks5tXZMFgaJpZM4RkbCf> .

[jboelen]

@eugensunic Looks like this was approved but not merged?
https://github.com/JedWatson/react-select/blob/master/src/Select.js#L205-L219

[eugensunic]

It was merged use 1.2.1 in package json and run npm install

…

On Feb 22, 2018 19:21, "James Boelen" ***@***.***> wrote: @eugensunic <https://github.com/eugensunic> Looks like this was approved but not merged? https://github.com/JedWatson/react-select/blob/master/src/ Select.js#L205-L219 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2314 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AK8OH160a8pPH3UteI5oskQQQ60uLkGtks5tXbAlgaJpZM4RkbCf> .

[jboelen]

Sorry if I'm missing something, but the commit that corresponds to version 1.2.1 shows that the lines remain unchanged.

react-select/lib/Select.js

Lines 251 to 265 in 82910d9

	value: function toggleTouchOutsideEvent(enabled) {
	if (enabled) {
	if (!document.addEventListener && document.attachEvent) {
	document.attachEvent('ontouchstart', this.handleTouchOutside);
	} else {
	document.addEventListener('touchstart', this.handleTouchOutside);
	}
	} else {
	if (!document.removeEventListener && document.detachEvent) {
	document.detachEvent('ontouchstart', this.handleTouchOutside);
	} else {
	document.removeEventListener('touchstart', this.handleTouchOutside);
	}
	}
	}

I've also checked the source from npm for v1.2.1, and it shows the same unchanged lines.

node_modules/react-select/dist/react-select.es.js

[vitorhorta]

@jboelen did you get any news on this PR? Still not merged here.

[abstract071]

I had trouble with closing select. The problem was that I set display: none to input element under react select. After removing this the behavior became ok.
Maybe it will be helpful for someone :)

[willmore]

@ytiurin despite comments stating this has been merged, it appears otherwise.

Is there an ETA on merging? Thank you.

[RocketPuppy]

We are also looking for this to be merged.

[abraham-chan]

ping @eugensunic PR approved but not merged/released?

[hle50]

we are waiting for this to be merged

[RealSilo]

Could you pls merge this?

[willmore]

@JedWatson appealing to you as the project owner. How do we get this merged in a "timely" manner? In general, what should be the expected time between reviewer approval merge? Thank you.

[JedWatson]

Responding specifically to @willmore's question, and please read my tone as constructive:

In general, what should be the expected time between reviewer approval merge?

I think you're talking about the "approval" by @eugensunic, and asking how long is reasonable to wait between that and the PR being merged and released?

To clarify, GitHub allows anyone to "Approve" a PR, regardless of their relationship to the project. But without the reviewer providing explanation or context for the core team it's effectively just a different kind of "thumbs up" from the community. I mean no offence, but this PR hasn't been reviewed or approved by anyone on the react-select team yet.

How do we get this merged in a "timely" manner?

Sorry this hasn't been dealt with yet - I've been spending a huge amount of time prepping v2 (see #2289) for final release and haven't had a lot of bandwidth for responding to v1 PRs and issues.

The things making this take longer include:

• Most of the discussion in the referenced issue is around confusion that the code in master hadn't been published in a version yet; that's been done, but the symptom of menu closing seems to still be active
• There are reports that the bug has indeed been solved, but not everybody agrees
• There are no clear instructions on how to reproduce, or linked codesandbox implementations demonstrating the issue
• There is no explanation with this PR about what the identified problem is or how this fixes it

So, explaining my view as a maintainer, this PR has no clarity or explanation. It's just a change to code and I'm not clear on what the change does or why it is important. I'm also not clear what the root cause of the reported issue is or how to reproduce it. From the comments in the linked issue, not everybody is experiencing it which means that any change needs to be careful to address all use-cases and not break anything else that's currently working for users.

If these things were clearer, I could more quickly resolve the issue and merge the PR or request changes.

In the meantime, it's in the backlog, and I'll get to it when I have time to dedicate to understanding the problem and proposed solution comprehensively.

[JedWatson]

I do realise that stronger guidelines for issues and PRs would help clarify why this is waiting, and result in less frustration and better quality contributions over all. I'll try and set that up in the future.

I'm also sure there are a lot of open issues and PRs that would benefit from this kind of clarity, and if anybody wants to help me manage that it would be really helpful.

[ytiurin]

Hi guys, more details about this commit from my side. The component menu closes on input blur, but doesn't close in case when input field is not used in the component. I think there was a code some time before, to handle no-input menu closing because we have this code for touch outside menu. So this commit handle situation when there is no input element visible on screen.

[willmore]

I'm also sure there are a lot of open issues and PRs that would benefit from this kind of clarity, and if anybody wants to help me manage that it would be really helpful.

Thank you for your comments @JedWatson! I will ask members of my team whether they are interested and able to contribute.

[jossmac]

Thanks @ytiurin 🙂

[TheSharpieOne]

This change appears to have broken the closeOnSelect={false} functionality of v1. since the mousedown listener will fire the handleTouchOutside and always determine that the target is not within the wrapper (since the target is removed from the DOM), it will always fire this.menuClose().