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

Reggino · 2017-02-22T23:05:01Z

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

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
- If I'm adding a new section, I've updated the Table of Content
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
I'm adding a new feature
- I've updated the playground with an example use of the feature

…fied

glasserc

Hi, thanks for your enthusiasm! I'm new to the project myself so I'll defer to @n1k0 on any review, but since he's probably asleep right now, here's what I noticed.

glasserc · 2017-02-22T23:25:40Z

src/utils.js

+
+    case "array":
+      if (schema.minItems) {
+        return new Array(schema.minItems).fill(schema.items.default);


Shouldn't this involve a recursive call to computeDefaults ? For example, what if the items are an object or have $ref ?

Thanks, fixed

glasserc · 2017-02-22T23:29:56Z

test/ArrayField_test.js

+      const {node} = createFormComponent({schema: complexSchema, formData: { }});
+
+      const inputs = node.querySelectorAll("input[type=text]");
+      expect(inputs.length).eql(4);


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?

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

n1k0

I checked the branch but couldn't make the new feature work in the playground. Am I missing something? Could you please update the playground with a sample use of it? Thanks!

n1k0 · 2017-02-23T08:45:06Z

src/utils.js

+
+    case "array":
+      if (schema.minItems) {
+        return new Array(schema.minItems).fill(schema.items.default);


Reggino · 2017-02-23T09:18:38Z

I've added an example to the playground.

n1k0 · 2017-02-23T09:20:43Z

test/ArrayField_test.js

+      // 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);


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.

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

n1k0 · 2017-02-23T09:21:33Z

Also, tested the patch again with the updated playground, works great!

n1k0 · 2017-02-23T09:23:59Z

src/utils.js

+      if (schema.minItems) {
+        return new Array(schema.minItems).fill(computeDefaults(schema.items, defaults, definitions));
+      }
+      return defaults || [];


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.

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

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

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?

True. I've updated the PR

Reggino

Should be ok now :-)

glasserc

Thanks!

n1k0 · 2017-02-24T07:16:28Z

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)

n1k0 · 2017-03-21T09:04:28Z

Released in v0.44.0.

rjsf-team#434 - Render empty array item fields when minItems is speci…

c66fbd6

…fied

glasserc reviewed Feb 22, 2017

View reviewed changes

n1k0 reviewed Feb 23, 2017

View reviewed changes

src/utils.js Outdated

case "array":

if (schema.minItems) {

return new Array(schema.minItems).fill(schema.items.default);

Copy link

Collaborator

n1k0 Feb 23, 2017

Choose a reason for hiding this comment

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

Indeed.

Reggino added 2 commits February 23, 2017 10:13

Added tests + playground feature for minItems

c171226

Lint issue

2f2a88b

n1k0 suggested changes Feb 23, 2017

View reviewed changes

n1k0 reviewed Feb 23, 2017

View reviewed changes

Reggino added 2 commits February 23, 2017 10:33

Split tests: 2 assertions = 2 tests

56950c7

Don't fall back to array

d23f03f

Reggino commented Feb 23, 2017

View reviewed changes

glasserc approved these changes Feb 24, 2017

View reviewed changes

glasserc merged commit 38a850b into rjsf-team:master Feb 24, 2017

glasserc mentioned this pull request Feb 24, 2017

Render empty array item fields when minItems is specified #434

Closed

matias-sandell mentioned this pull request Apr 6, 2017

Adding default empty array items when initializing a form #134

Closed

dmethvin-gov mentioned this pull request Jul 6, 2018

Remove react-jsonschema-form dependency usds/us-forms-system#141

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Reggino commented Feb 22, 2017 •

edited

Loading

glasserc left a comment

glasserc Feb 22, 2017

n1k0 Feb 23, 2017

Reggino Feb 23, 2017 •

edited

Loading

glasserc Feb 22, 2017

Reggino Feb 23, 2017

n1k0 left a comment •

edited

Loading

n1k0 Feb 23, 2017

Reggino commented Feb 23, 2017

n1k0 Feb 23, 2017

Reggino Feb 23, 2017

n1k0 commented Feb 23, 2017

n1k0 Feb 23, 2017

Reggino Feb 23, 2017

Reggino Feb 23, 2017

n1k0 Feb 23, 2017

Reggino Feb 23, 2017

Reggino left a comment

glasserc left a comment

n1k0 commented Feb 24, 2017

n1k0 commented Mar 21, 2017

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

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

Conversation

Reggino commented Feb 22, 2017 • edited Loading

Reasons for making this change

Checklist

glasserc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reggino Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1k0 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reggino commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1k0 commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reggino left a comment

Choose a reason for hiding this comment

glasserc left a comment

Choose a reason for hiding this comment

n1k0 commented Feb 24, 2017

n1k0 commented Mar 21, 2017

Reggino commented Feb 22, 2017 •

edited

Loading

Reggino Feb 23, 2017 •

edited

Loading

n1k0 left a comment •

edited

Loading