-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Changes from 3 commits
c66fbd6
c171226
2f2a88b
56950c7
d23f03f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", () => { | ||
|
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.
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.
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
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.
BTW when no value would be defined at all, that would lead to
[]
due to thedefaultProps
in ArrayField.jsThere 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 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.
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