Skip to content

Conversation

@huhuaaa
Copy link

@huhuaaa huhuaaa commented Feb 2, 2019

Fixed the bug. The selector have an empty option, when use enum and the default value is 0 or false or ''.

Reasons for making this change

[Please describe them here]

Fixed the bug. The selector have an empty option, when use enum and the default value is 0 or false or ''.

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@epicfaace
Copy link
Member

epicfaace commented Feb 4, 2019

@huhuaaa Can you explain what issue you are solving? When the default value is 0 or false or "", what do you expect the dropdown to look like?

When the default value is "" or false or "" for me, it looks like this (playground link):
image

@huhuaaa
Copy link
Author

huhuaaa commented Feb 11, 2019

Yes, i use "typeof default === 'undefined'" to check the default value.

@epicfaace
Copy link
Member

Why would we not want to have an empty option when the default is 0 or false or ''?

@huhuaaa
Copy link
Author

huhuaaa commented Feb 11, 2019

Yes, when the default is 0 or false or '' , we need an option. But i think an empty option is not the best one. And there's another bug here,i set enum = [0,1] and enumNames = ['zero', 'one'] , the empty option also showing. This result is not expected.

code sample

image

@epicfaace
Copy link
Member

Oh, I see, so this is solving #1177

@epicfaace
Copy link
Member

@huhuaaa Can you add some tests for this?

@huhuaaa
Copy link
Author

huhuaaa commented Feb 12, 2019

OK

@huhuaaa
Copy link
Author

huhuaaa commented Feb 12, 2019

I had commited the tests.

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Thanks for the tests -- can you add that additional test I mentioned? Additionally, rather than making a separate file, I think you should add those tests in this file: https://github.com/mozilla-services/react-jsonschema-form/blob/master/test/NumberField_test.js#L244

expect(options[0].innerHTML).eql("0");
});

it("should render a select element and it's first option is 'false', if set the enum and the default value is false.", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a test in which the default value is '', and then it shouldn't render two empty options?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your suggestions, i have adjusted it.

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