Skip to content

Fix backspace handling for non-multi select controls. Fixes #638 #773

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.

Sign up for GitHub

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

Jump to bottom
Merged
agirton merged 1 commit into from
Jun 14, 2017
Merged

Fix backspace handling for non-multi select controls. Fixes #638 #773

agirton merged 1 commit into from
Jun 14, 2017
+14 −1

Conversation

@MrLeebo
Copy link
Contributor

@MrLeebo MrLeebo commented Feb 16, 2016 •
edited
Loading

Merging this PR will resolve the following issues: #638, #746, #1596, #1680, #1215

REPRO STEPS:

  1. npm start
  2. open http://localhost:8000
  3. Scroll down to "Contributors (Async)". This issue impacts components with the following props: <Select multi={false} simpleValue={false} />
  4. Check "Single Value"
  5. Change the dropdown selection
  6. Press backspace

EXPECTED:

screen shot 2017-06-13 at 11 56 08 am

ACTUAL:

screen shot 2017-06-13 at 11 54 26 am

RESOLUTION:

Non-multi select controls return an empty array in the onChange event instead of the expected null value when you press backspace to clear the selection.

This is @slestang's implementation from #638 and I've added a unit test.

@MrLeebo
Copy link
Contributor Author

MrLeebo commented Feb 16, 2016

@JedWatson I don't think the CI failure has anything to do with this commit. The test 'should filter after entering some text' wouldn't be impacted by this change and seems like a long-running test to begin with. When I run tests locally that test passes but takes 487ms to run, whereas all other tests are < 20 ms.

Since the CI server looks like a less powerful machine, the test is probably just hitting the 2s timeout.

Weird that a nearly identical test like should filter case insensitively runs much faster. Maybe something is getting cached?

@MrLeebo MrLeebo mentioned this pull request Feb 18, 2016
Closed
@hanc1208
Copy link

hanc1208 commented Oct 7, 2016

Please re-run ci and merge this pull-request..

@MrLeebo MrLeebo closed this Oct 7, 2016
@MrLeebo MrLeebo reopened this Oct 7, 2016
@coveralls
Copy link

coveralls commented Oct 7, 2016

Coverage Status

Coverage increased (+1.1%) to 92.617% when pulling 74960fd on MrLeebo:backspace-placeholder into 5cdfaed on JedWatson:master.

@MrLeebo
Copy link
Contributor Author

MrLeebo commented Oct 7, 2016

@JedWatson @hanc1208 Closing and reopening the PR resolved the CI issue. I don't know why I didn't think to do that earlier :)

@hanc1208
Copy link

hanc1208 commented Oct 8, 2016

@MrLeebo Thank you!

@kenjiru
Copy link

kenjiru commented Dec 20, 2016

Please merge this pull request!

1 similar comment
@tproenca
Copy link

tproenca commented Feb 9, 2017

Please merge this pull request!

This was referenced Mar 10, 2017
Closed
Closed
@dongwoo1005
Copy link

dongwoo1005 commented Apr 4, 2017

Is this PR ever getting merged?

@bluespore
Copy link

bluespore commented Apr 10, 2017

@JedWatson - I've implemented a soft fix in the mean time, but it looks as though there are others that have been waiting for this one to be merged as well. Gentle bump on an update here :)

@julien-f julien-f mentioned this pull request Apr 27, 2017
Closed
@nealoke
Copy link

nealoke commented May 31, 2017

Please merge 👍

@MrLeebo
Copy link
Contributor Author

MrLeebo commented Jun 13, 2017

@JedWatson @agirton I've updated the description with steps to reproduce the issue in master. The PR resolves this issue. Please let me know if there is anything else that needs to happen for this to be merged.

@agirton
Copy link
Collaborator

agirton commented Jun 14, 2017

Hi @MrLeebo thank you for your patience and the unit test. Agreed this should be merged in.

@agirton agirton merged commit 40a9eb2 into JedWatson:master Jun 14, 2017
@MrLeebo MrLeebo deleted the backspace-placeholder branch June 14, 2017 04:37
ryanabooth pushed a commit to entelo/react-select that referenced this pull request Jun 29, 2017
Merge commit '26169305a721ec3099a912cea2f6ed38e6dc9c4c' into ryan/upd…
bcc482c 
…ate_and_add_creatable_options

* commit '26169305a721ec3099a912cea2f6ed38e6dc9c4c': (36 commits)
  Fix Usage Docs Example (JedWatson#1799)
  Hide create option after closing menu (JedWatson#1306)
  Update README.md to include 'valueComponent' prop (JedWatson#1803)
  Adding merged changes to changelog
  Fix backspace handling for non-multi select controls. Fixes JedWatson#638 (JedWatson#773)
  Update Select.js
  [ADD] Clarified the onInputChange prop signature in the docs (JedWatson#1773)
  [FIX] JedWatson#1651 moved option prop sync to componentWillReceiveProps (JedWatson#1765)
  v1.0.0-rc.5
  Updating build
  Update changelog
  Fix selected option focus when valueKey is not "value" (JedWatson#1733)
  Fix aria owns value (JedWatson#1556)
  Fix JedWatson#1394 (JedWatson#1395)
  v1.0.0-rc.4
  Bumping create-react-class and prop-types
  Update react-input-autosize dep
  added variables for shadow styles
  Move over examples to use new pkgs
  Migrate rest of src overto createClass pkg
  ...

# Conflicts:
#	dist/react-select.js
#	dist/react-select.min.js
#	examples/dist/bundle.js
#	examples/dist/standalone.js
#	lib/Async.js
#	lib/Value.js
@cssecautomation cssecautomation mentioned this pull request Feb 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@MrLeebo @hanc1208 @coveralls @kenjiru @tproenca @dongwoo1005 @bluespore @nealoke @agirton