Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jul 16, 2015

Fixes #318.

@JedWatson
Copy link
Owner

@NSinopoli would you mind helping out by explaining how this fixes the problem?

@bruderstein
Copy link
Collaborator

I can confirm that
a) The problem exists, and is easy to replicate
b) This fixes it.

However, I can't yet replicate the double onChange issue in a test. I can replicate that existing selections aren't removed when searchable=false, and this fixes that too (and that the test then passes)

I'll keep on it - when I can replicate it in a test, I should be able to see what causes it, and therefore why this fixes it.

@bruderstein
Copy link
Collaborator

I'm a little bit further - leaving this here in case anyone has any ideas:

The mouseDown event on the option is fired twice, but on two different "instances", ie. two different renders.

mouseDown fires, adds the value, calls setState, rerenders, render calls buildMenu(),
mouseDown fires on the re-rendered instance of the option.

It "feels" like a timing issue, as it's not always, but is mostly reproducible for me on the selection of the second item, but occasionally it works, and it works if I step it through via the debugger.

@bruderstein
Copy link
Collaborator

I'm basically giving up :(

This fixes the issue because the item isn't there after the first mouseDown for the second mouseDown event to fire. Why it fires twice is still a mystery to me. I've tried rebuilding a little example with the same idea - regenerating options with mouseDown, mouseEnter and mouseLeave events that call setState, even tried making render() slow, and everything works perfectly.

I think what this really needs, is a bit of a refactor so that the menu is a separate component. This could potentially simplify the main component.

TL;DR: I'm 👍 for this to be merged :)

bruderstein added a commit to bruderstein/react-select that referenced this pull request Aug 2, 2015
Adds tests for removing existing selection from options
This will not pass until PR JedWatson#319 is merged
bruderstein added a commit to bruderstein/react-select that referenced this pull request Aug 3, 2015
Adds tests for removing existing selection from options
This will not pass until PR JedWatson#319 is merged
@bruderstein bruderstein mentioned this pull request Aug 3, 2015
bruderstein added a commit to bruderstein/react-select that referenced this pull request Aug 5, 2015
Adds tests for removing existing selection from options
This will not pass until PR JedWatson#319 is merged
JedWatson added a commit that referenced this pull request Aug 5, 2015
Filter options regardless of searchable setting
@JedWatson JedWatson merged commit 9b35b8b into JedWatson:master Aug 5, 2015
@JedWatson
Copy link
Owner

Thanks for the review @bruderstein - happy to go with your 👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants