#434 - Render empty array item fields when minItems is specified #484
Conversation
src/utils.js
Outdated
|
|
||
| case "array": | ||
| if (schema.minItems) { | ||
| return new Array(schema.minItems).fill(schema.items.default); |
There was a problem hiding this comment.
Shouldn't this involve a recursive call to computeDefaults ? For example, what if the items are an object or have $ref ?
test/ArrayField_test.js
Outdated
| const {node} = createFormComponent({schema: complexSchema, formData: { }}); | ||
|
|
||
| const inputs = node.querySelectorAll("input[type=text]"); | ||
| expect(inputs.length).eql(4); |
There was a problem hiding this comment.
Should we test that the right default value is chosen? I see that there's no other tests for the "default" behavior for ArrayField, so this might be excessive.
Should we test that merging formData with defaults works correctly?
There was a problem hiding this comment.
IMHO formData and defaults can really be merged in case of an array: it is either set or not set. When set, the form should honor the given value. When not set it may compute a sane default.
I've added a test for this behaviour
src/utils.js
Outdated
|
|
||
| case "array": | ||
| if (schema.minItems) { | ||
| return new Array(schema.minItems).fill(schema.items.default); |
|
I've added an example to the playground. |
test/ArrayField_test.js
Outdated
| // whenever a value is set, honor it! | ||
| form = createFormComponent({schema: complexSchema, formData: {foo: []}}); | ||
| inputs = form.node.querySelectorAll("input[type=text]"); | ||
| expect(inputs.length).eql(0); |
There was a problem hiding this comment.
It would be better to distinguish between the two sets of assertions here, so we're better informed when one of these fails.
This can usually be achieved by either mounting a single component per it test.
There was a problem hiding this comment.
You're completely right. I've updated the PR
|
Also, tested the patch again with the updated playground, works great! |
src/utils.js
Outdated
| if (schema.minItems) { | ||
| return new Array(schema.minItems).fill(computeDefaults(schema.items, defaults, definitions)); | ||
| } | ||
| return defaults || []; |
There was a problem hiding this comment.
Why do we fallback to an empty array here? I understand it may be confusing but we need to distinguish between an empty array and no array value defined at all. I'd suggest we just return defaults here.
There was a problem hiding this comment.
IMHO computeDefaults would ideally be the only place where defaults are "made up". I actually tried to remove this line:
https://github.com/mozilla-services/react-jsonschema-form/blob/master/src/components/fields/ArrayField.js#L153
But other tests would break. However, it would be more clear when all defaults would originate in computeDefaults
There was a problem hiding this comment.
BTW when no value would be defined at all, that would lead to [] due to the defaultProps in ArrayField.js
There was a problem hiding this comment.
I agree, we'll have to work to make this more consistent in the future. Meanwhile here, I'm pretty sure we could remove the whole return defaults || []; line without breaking anything, as that would match previous behavior, don't you think?
There was a problem hiding this comment.
True. I've updated the PR
|
Thanks++ |
New features * Add support for rows attribute of textarea widget. (#450) * #434 - Render empty array item fields when minItems is specified (#484) * Add a "has-danger" class to the form error list (#502) * Show description for boolean fields (#498) * Fix #488: Add a custom Form ErrorList prop. Bugfixes * Fix impossibility to use stateful ArrayFieldTeplate comp. (#519) * Centralized shouldComponentUpdate handling in SchemaField (#490)
|
Released in v0.44.0. |
Reasons for making this change
Trying to get my feet wet with this library and doing something useful on the way 😄
Implemented #434
BTW thanks for this awesome stuff!
Checklist