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

Fix first option if disabled (#473) #583

Closed
wants to merge 4 commits into from

Conversation

markjoeljimenez
Copy link

Description

Fix issue #473.
The first disabled choice passed in as an option is not actually disabled.

Reproduced example

https://jsfiddle.net/uf9bmpy6/

Fix

We can simply pass in the disabled choice instead of checking whether it's preselected or not.

@jshjohnson
Copy link
Collaborator

Thanks for this @markjoeljimenez. Could you add some tests to cover this change please?

@markjoeljimenez
Copy link
Author

Hey @jshjohnson, I haven't written tests before so please let me know if my tests are sufficient enough.

@bronzehedwick bronzehedwick mentioned this pull request Sep 30, 2019
6 tasks
.click();
});

it('first choice is disabled', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('first choice is disabled', () => {
it('disables the first choice', () => {


let selectValue;

it('first non-disabled choice is selected', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('first non-disabled choice is selected', () => {
it('selects the first non-disabled choice', () => {

@@ -230,6 +236,29 @@ <h2>Select one inputs</h2>
removeItemButton: true,
});

new Choices('#choices-disabled-first-choice-by-options', {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
new Choices('#choices-disabled-first-choice-by-options', {
new Choices('#choices-disabled-choice-via-options', {

Copy link
Author

@markjoeljimenez markjoeljimenez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests

@jshjohnson jshjohnson added the bugfix Pull request that fixes an existing bug label Oct 30, 2019
@jshjohnson jshjohnson mentioned this pull request Nov 4, 2019
8 tasks
@jshjohnson
Copy link
Collaborator

Hey - i've opened a new PR with your fix + some tweaks over latest master. Thanks 👍

@jshjohnson jshjohnson closed this Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants