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

Don't open drop down menu when clear values #2198

Merged
merged 2 commits into from
Dec 14, 2017
Merged

Don't open drop down menu when clear values #2198

merged 2 commits into from
Dec 14, 2017

Conversation

Chopinsky
Copy link
Contributor

This PR is to address issue #1252, where when "clear" button is clicked upon, the drop down menu will prompt to open. This issue is caused by involuntarily firing the focus event after clearing values, where we will prompt the menu regardless of the context.

To address this issue, we will set a flag in the clearValue function, and then reset the flag after the focus handler finishes.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 93.497% when pulling 63f1d10 on Chopinsky:fix/ClearToOpen into 62cdd5d on JedWatson:master.

@garrettdehnketurpin
Copy link

Just curious, on when this branch will be merged?

Regards,
Garrett

@JedWatson
Copy link
Owner

Opening when the clear button is clicked was originally intended behaviour, but having tested the two this does make more sense. Seems like if the user intention is to clear the field, it doesn't necessarily also convey an intention to open the menu... right?

This also makes the "clear" behaviour consistent with the behaviour of the "remove" icon on values in a multi-select, so I'm happy to merge it.

Would you mind resolving the conflict @Chopinsky? I've taken a look and am not 100% confident how to preserve your fix without digging deeper into it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 93.813% when pulling fbe0567 on Chopinsky:fix/ClearToOpen into 700b79a on JedWatson:master.

@JedWatson
Copy link
Owner

Nevermind, I'm about to release a version and want to include this so I've dug in deeper and resolved the conflicts. Thanks for the PR @Chopinsky!

@JedWatson JedWatson merged commit 28f6afb into JedWatson:master Dec 14, 2017
@Chopinsky Chopinsky deleted the fix/ClearToOpen branch December 14, 2017 14:56
@samuelcolvin
Copy link

Any chance of getting a release with this included?

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.

5 participants