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

Add unselected option to select widget when no default is set #673

Merged
merged 1 commit into from
Nov 11, 2017

Conversation

Benaiah
Copy link
Contributor

@Benaiah Benaiah commented Oct 10, 2017

- Summary

Add a "not selected" option to the select widget when there is no default set. Closes #562.

- Test plan

Tested manually.

- Description for the changelog

Add unselected option to select widget when no default is set.

- A picture of a cute animal (not mandatory but encouraged)

image

Copy link
Contributor

@tech4him1 tech4him1 left a comment

Choose a reason for hiding this comment

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

LGTM, except one thing.

return option;
});
const options = [
...(field.get('default', false) ? [] : [{ label: '<not set>', value: '' }]),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the <not set> field should be a regular option -- I don't think it should be selectable. After an option has been selected, the <not set> field should be hidden.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would change { label: '<not set>', value: '' } to { label: '<not set>', disabled: true }.

Copy link
Contributor Author

@Benaiah Benaiah Oct 10, 2017

Choose a reason for hiding this comment

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

@tech4him1 good point, I'll make the change. It's always possible to allow an empty value by explicitly adding it to the options.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Benaiah actually, now that you say that I'm not sure it does. If I use this PR like it is right now, it won't let me save if I select <not set>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tech4him1 "not sure it does" 👈 not sure it does what?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Benaiah There is no way, AFAIK, to save a select with an empty value unless required: false. Is that what you meant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tech4him1 I meant that if you want to save an empty string, you can always add an option like { label: "Empty", value: "" } to the config. <not set> shouldn't be selectable, as you suggested.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

@Benaiah Benaiah force-pushed the fix-select-widget-unselected-state branch from 337d876 to 0de01e1 Compare November 10, 2017 00:49
@Benaiah
Copy link
Contributor Author

Benaiah commented Nov 10, 2017

@tech4him1, @erquhart this is finished and r2r.

@erquhart erquhart dismissed tech4him1’s stale review November 11, 2017 15:33

His review was LGTM with a caveat, caveat addressed.

@erquhart erquhart merged commit e00c396 into master Nov 11, 2017
@erquhart erquhart deleted the fix-select-widget-unselected-state branch November 11, 2017 15:33
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.

Select widget displays first option by default, but behaves as if empty
3 participants