Add Checkbox (boolean) field type #147
Conversation
|
Note, this is related to issue #19 |
There was a problem hiding this comment.
Looks good! A couple of questions noted.
Big change request: please rename this field type to Checkbox because Boolean is already a literal in JavaScript.
| } | ||
| } | ||
|
|
||
| module.exports = { |
There was a problem hiding this comment.
Why not just export class ... above?
There was a problem hiding this comment.
Just following the convention defined in the other field types. I'm guessing this is because the file is used in a node context as well as a webpack-ified context.
| } from '@keystonejs/ui/src/primitives/fields'; | ||
|
|
||
| // TODO: use pretty checkboxes - these only work in a CheckGroup situation. | ||
| // import { Checkbox } from '@keystonejs/ui/src/primitives/forms'; |
There was a problem hiding this comment.
@jossmac this looks like a job for you
| return ( | ||
| <select onChange={this.handleChange} innerRef={innerRef} value={value}> | ||
| <option value="true">true</option> | ||
| <option value="false">false</option> |
There was a problem hiding this comment.
is it worth adding support for null (label "not set") here?
packages/test-project/index.js
Outdated
| @@ -72,6 +73,7 @@ keystone.createList('User', { | |||
| // TODO: Create a Twitter field type to encapsulate these | |||
| twitterId: { type: Text }, | |||
| twitterUsername: { type: Text }, | |||
| isAdmin: { type: Boolean, defaultValue: true }, | |||
There was a problem hiding this comment.
We don't support defaultValue yet - please don't include it in the test project until we do. Only stuff that's actually working belongs in here, otherwise it's not clear what's implemented and what's not.
There may be other exceptions to this rule already in place... if they are, we need to clean it up.
|
👍 |
mikehazell
force-pushed
the
add-boolean-field-type
branch
2 times, most recently
from
d0df40d to
15ebaae
Compare
mikehazell
force-pushed
the
add-boolean-field-type
branch
from
15ebaae to
ba1e25a
Compare
|
LGTM |
- renamed to “Checkbox” - added “Not set” as a filter option -
mikehazell
force-pushed
the
add-boolean-field-type
branch
from
ba1e25a to
e85cbc8
Compare
resolves #146
Adds the Checkbox core field type