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

#434 - Render empty array item fields when minItems is specified #484

Merged
merged 5 commits into from
Feb 24, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion playground/samples/arrays.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,16 @@
module.exports = {
schema: {
definitions: {
Thing: {
type: "object",
properties: {
name: {
type: "string",
default: "Default name"
}
}
}
},
type: "object",
properties: {
listOfStrings: {
Expand All @@ -15,7 +26,7 @@ module.exports = {
title: "A multiple choices list",
items: {
type: "string",
enum: ["foo", "bar", "fuzz", "qux"],
enum: ["foo", "bar", "fuzz", "qux"]
},
uniqueItems: true
},
Expand All @@ -38,6 +49,14 @@ module.exports = {
type: "number"
}
},
minItemsList: {
type: "array",
title: "A list with a minimal number of items",
minItems: 3,
items: {
$ref: "#/definitions/Thing"
}
},
nestedList: {
type: "array",
title: "Nested list",
Expand Down
26 changes: 17 additions & 9 deletions src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,15 +123,23 @@ function computeDefaults(schema, parentDefaults, definitions={}) {
if (typeof(defaults) === "undefined") {
defaults = schema.default;
}
// We need to recur for object schema inner default values.
if (schema.type === "object") {
return Object.keys(schema.properties).reduce((acc, key) => {
// Compute the defaults for this node, with the parent defaults we might
// have from a previous run: defaults[key].
acc[key] = computeDefaults(
schema.properties[key], (defaults || {})[key], definitions);
return acc;
}, {});

switch (schema.type) {
// We need to recur for object schema inner default values.
case "object":
return Object.keys(schema.properties).reduce((acc, key) => {
// Compute the defaults for this node, with the parent defaults we might
// have from a previous run: defaults[key].
acc[key] = computeDefaults(
schema.properties[key], (defaults || {})[key], definitions);
return acc;
}, {});

case "array":
if (schema.minItems) {
return new Array(schema.minItems).fill(computeDefaults(schema.items, defaults, definitions));
}
return defaults || [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW when no value would be defined at all, that would lead to [] due to the defaultProps in ArrayField.js

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I've updated the PR

}
return defaults;
}
Expand Down
35 changes: 35 additions & 0 deletions test/ArrayField_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,41 @@ describe("ArrayField", () => {
expect(inputs[2].id).eql("root_foo_1_bar");
expect(inputs[3].id).eql("root_foo_1_baz");
});

it("should render enough inputs with proper defaults to match minItems in schema when no formData is set", () => {
const complexSchema = {
type: "object",
definitions: {
Thing: {
type: "object",
properties: {
name: {
type: "string",
default: "Default name"
}
}
}
},
properties: {
foo: {
type: "array",
minItems: 2,
items: {
$ref: "#/definitions/Thing"
}
}
}
};
let form = createFormComponent({schema: complexSchema, formData: { }});
let inputs = form.node.querySelectorAll("input[type=text]");
expect(inputs[0].value).eql("Default name");
expect(inputs[1].value).eql("Default name");

// 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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're completely right. I've updated the PR

});
});

describe("Multiple choices list", () => {
Expand Down