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

Add test fixtures to cover disabled selected options #10142

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@nhunzaker
Collaborator

nhunzaker commented Jul 11, 2017

Related issues: #2803

We don't have amazing test fixture coverage for selects, so I've added two cases based on the discussion in #2803:

  1. A selected, disabled, option should be picked, but not selectable
  2. When there is no value match, the default selected option should be the first non-disabled option

Pretty standard stuff. A lot of this is just basic browser behavior, but I believe these test cases already caught a bug in the current edge build of React:

Selects no longer "skip" over disabled options when selecting the default option. (Example of standard DOM behavior)

Also, I've added a new field to link related issues in the test cases. Take a look at the result here:

http://react-select-default-option-fixtures.surge.sh/selects

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 11, 2017

Collaborator

@aweary Would it be possible to check this out on your local React to make sure I'm not crazy? It looks like the behavior for selects has changed between 16.0.0-alpha3 and the local build.

16.0.0-alpha3

The default value is "0" for the last test case:

screen shot 2017-07-11 at 8 22 04 am

Local Build

The default value is "Please select an option":

screen shot 2017-07-11 at 8 22 41 am

Collaborator

nhunzaker commented Jul 11, 2017

@aweary Would it be possible to check this out on your local React to make sure I'm not crazy? It looks like the behavior for selects has changed between 16.0.0-alpha3 and the local build.

16.0.0-alpha3

The default value is "0" for the last test case:

screen shot 2017-07-11 at 8 22 04 am

Local Build

The default value is "Please select an option":

screen shot 2017-07-11 at 8 22 41 am

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Aug 14, 2017

Member

@nhunzaker I'm seeing the same thing here, with the latest master build. This does indeed look like a regression. I'll take a look at it.

Member

aweary commented Aug 14, 2017

@nhunzaker I'm seeing the same thing here, with the latest master build. This does indeed look like a regression. I'll take a look at it.

@aweary aweary added this to the 16.0 milestone Aug 14, 2017

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Aug 14, 2017

Member

This issue stems from #8349 which introduced an updateOptions call when a select element is mounted. IfupdateOptions doesn't find a matching option it defaults to setting the first option as selected.

In previous versions updateOptions was not called on the initial mount, so it defaulted to the standard behavior of the browser.

Member

aweary commented Aug 14, 2017

This issue stems from #8349 which introduced an updateOptions call when a select element is mounted. IfupdateOptions doesn't find a matching option it defaults to setting the first option as selected.

In previous versions updateOptions was not called on the initial mount, so it defaulted to the standard behavior of the browser.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 14, 2017

Member

cc @sebmarkbage regarding above explanation

Member

gaearon commented Aug 14, 2017

cc @sebmarkbage regarding above explanation

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 1, 2017

Member

Seems like #10456 superseded this.

Member

gaearon commented Sep 1, 2017

Seems like #10456 superseded this.

@gaearon gaearon closed this Sep 1, 2017

@bvaughn bvaughn referenced this pull request Sep 7, 2017

Closed

React 16 RC #10294

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