-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
There was a problem hiding this 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: '' }]), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 }
.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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>
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
337d876
to
0de01e1
Compare
@tech4him1, @erquhart this is finished and r2r. |
His review was LGTM with a caveat, caveat addressed.
- 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)