Enable Arrays to be marked as required, and fixes for strings marked as required #442

crumblix · 2017-01-18T01:42:05Z

Reasons for making this change

New feature:

Enable the arrays to be marked as "required". Meaning that the user must select "at least one" option from the list.

This references the array part of ticket:
#411

It also refers to the string part of
#410 and #402

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

MoOx · 2017-01-18T07:08:58Z

README.md

@@ -558,6 +558,8 @@ const uiSchema = {
 };
 ```

+In addition it is valid to maked an array as "required".  This means the user must select at least one items to add to the array.


to maked? Looks like a typo. Maybe "to mark"?

crumblix · 2017-01-18T07:51:22Z

Nice pickup ... thank you!!! :)

n1k0

Still some heavy questions, and a few nits here and there.

Also wondering about <SelectWidget>, what should we do with enums containing an empty string when it's selected?

n1k0 · 2017-01-18T08:05:32Z

README.md

@@ -558,6 +558,8 @@ const uiSchema = {
 };
 ```

+In addition it is valid to mark an array as "required".  This means the user must select at least one item to add to the array.


I'm realizing slightly late that this is overlapping with the minItems jsonschema directive. I think the jsonschema spec wants a required object array property to be an array, possibly an empty one. With jsonschema, if we wanted at least one item in our array, we'd set minItems to 1 instead of relying on the required directive.

So that's a strong direction we're taking here, we're officially diverging from the jsonschema spec to fit Web forms use cases & habits. Is everybody okay with this?

Would appreciate inputs from @MoOx @maartenth @olzraiti @leplatrem @Natim @dehli

Fair call. I wasn't aware of that directive. If we can achieve a similar effect with existing code that is standards compliant I would certainly support that.

Indeed that's a serious diff. Not sure I am comfortable with that but I guess we can't have the silver bullet.

The SelectWidget is an interesting case. In truth I don't use it much because I've written my own custom widget based on react-select which better suited my needs for a nullable and multiselect-able drop down widget. (Happy to share if anyone wants it, but I thought since it was an extra dependency it was better off outside the core project.)

I could be convinced to do it here as well, since of course the "required" tag won't work, and while I prefer react-select for this case, perhaps like dates we possibly should cater for the most general case.

This wouldn't affect my code base, but I'm not sure if we would benefit enough from adding this considering the consequences.

We aren't extending the json schema spec in a way that would make something possible that was impossible before (using minItem provides the functionality that is wanted here). We are merely making something more convenient than before, with the cost of breaking compatibility with json schema spec.

JSON Schema is not a form builder

The issue is that people import existing schemas and expect them to work with forms with no need to tweak their schemas (uiSchemas have been introduced for that specific purpose).

That could be addressed with an appropriate FAQ/Caveats section though. The same way we could document the minItems jsonschema directive.

the difference between an empty array and undefined is simply not visible in a form

That's indeed a fair point. Another approach would be to make the difference actually visible, but that's rather tricky to get right, and would probably clutter UX a lot. Also this would have to work with bootstrap and other styles.

So my current thinking is:

Keep the empty string as undefined value feature from this patch.

Remove the required array should contain at least one item one and add proper docs describing how to achieve it using the minItems jsonschema directive.

Leave SelectWidget as it is, users should provide enum values matching their expectations.

What do people think?

I agree with you @n1k0 . I'm currently using minItems in my application b/c that's the json-schema way of doing it. If we added documentation describing it, then it'll be more obvious that it's the way to do it.

Sounds good to me! Empty string as undefined seems well-grounded.

I'm OK with your proposed solution, @n1k0. I think it is an improvement to the current situation either way.

Good, @crumblix would you mind updating the patch accordingly if you agree with this decision? :)

Thanks a bunch 👍

n1k0 · 2017-01-18T10:20:36Z

src/components/fields/ArrayField.js

      this.asyncSetState({
-        items: this.state.items.filter((_, i) => i !== index)
+        items: newitems
      }, {validate: true}); // refs #195


const items = this.state.items.filter((_, i) => i !== index); this.asyncSetState({ items: items.length > 0 ? items : undefined, }, {validate: true}); // refs #195

n1k0 · 2017-01-18T10:25:18Z

src/components/widgets/BaseInput.js

+        return _onChange(undefined);
+      } else {
+        return _onChange(event.target.value);
+      }}}/>


const _onChange = ({target: {value}}) => { return props.onChange(value === "" ? undefined : value); }; // ... onChange={_onChange} />

n1k0 · 2017-01-18T10:26:57Z

src/components/widgets/CheckboxesWidget.js

+    return undefined;
+  } else {
+    return ret;    
+  }  


const ret = selected.filter(v => v !== value); return ret.length === 0 ? undefined : ret;

n1k0 · 2017-01-18T10:28:35Z

src/components/widgets/CheckboxesWidget.js

@@ -19,7 +24,7 @@ function CheckboxesWidget(props) {
  return (
    <div className="checkboxes" id={id}>{
      enumOptions.map((option, index) => {
-        const checked = value.indexOf(option.value) !== -1;
+        const checked = (value === undefined) ? false : value.indexOf(option.value) !== -1;


Nit: Unnecessary wrapping parenthesis.

n1k0 · 2017-01-18T10:29:13Z

src/components/widgets/TextareaWidget.js

+        return _onChange(undefined);
+      } else {
+        return _onChange(event.target.value);
+      }}}/>


See previous impl. suggestion above.

crumblix · 2017-01-18T10:56:46Z

Thank you very much for the code review! Much appreciated, I have no problem addressing a few nits :)

dehli · 2017-01-18T19:44:24Z

src/components/fields/ArrayField.js

@@ -216,8 +216,9 @@ class ArrayField extends Component {
      if (event) {
        event.preventDefault();
      }
+      var items = this.state.items.filter((_, i) => i !== index);


I think this should be a const or let. It looks like you never assign to it again, so a const would be the way to go.

crumblix · 2017-01-19T22:52:07Z

Thanks again everyone for the feedback :)
I reverted the documentation and fixed the last nit to make the code cleaner and better.

I'll need to read up on minItems before making any changes to the documentation for that as I haven't used it before.

My plan is to try and get it working in the playground later before updating the documentation about it.

Either that or if someone who knows how it works already sends me a sentence I'll just include it.

…with minItems.

crumblix · 2017-01-20T01:32:21Z

OK, so I've reverted the array changes, played around with minItems in the playground, and tried to make it clear how to achieve a similar (and more feature rich) result by using it in the documentation.

Thanks for pointing out minItems!

n1k0

We're getting there! Could you please add two simple tests for baseinput and textarea widgets to ensure they propagate undefined when their change event value is set to ""? Thank you!

n1k0 · 2017-01-20T07:27:27Z

README.md

@@ -547,6 +547,23 @@ const uiSchema = {
 };
 ```

+Unlike other types, array's do not support the `required` property.  You can however instead specify the minimum number of items the user must select with the `minItems` property.


It actually supports the required property, it only checks that an array property value is provided, which may or may not be empty. Put another way, for

schema = { type: "object", required: ["foo"], properties: { foo: {type: "array"} } }

{foo: []} is valid against this schema while {} is not.

No problem! While it's true supports the required property, I suspect that it doesn't operate in the fashion most UI builders would expect, at the very least it's a potential source of confusion. I'd try and update the documentation to be clear in that regard. :)

And yes to the tests as well! :)

I suspect that it doesn't operate in the fashion most UI builders would expect

I think we need to stay true to the jsonschema spec as much as possible, as it's our data model layer behind our form views. One of the goal of this lib is also to make forms and their underlying data model more robust and consistent. I wouldn't want to bring regular web forms flakiness to jsonschema, which would kind of defeat its purpose if you ask me :)

The best patch for preventing confusion is good documentation.

…equired and minItems properties when used in arrays.

n1k0 · 2017-01-20T07:55:05Z

README.md

@@ -547,7 +547,7 @@ const uiSchema = {
 };
 ```

-Unlike other types, array's do not support the `required` property.  You can however instead specify the minimum number of items the user must select with the `minItems` property.
+Care should be taken when using the `required` property with arrays.  An empty array is sufficient to pass that validation check.  If you wish to ensure the user populates the array, you can specify the minimum number of items the user must select with the `minItems` property.


This is perfect 👍

Excellent :)
I'll have to get to the tests over the weekend. It's a part of the code base I haven't been into yet.

Unfortunately there wasn't a text area widget section that I could see so I created one .... but it isn't right. I will have to look at it over the weekend.

What's checked in is passing, but it's not using the widget.

I've tried the change:

const uiSchema = {"ui:widget": "textarea"}; it("should handle an empty string change event", () => { const {comp, node} = createFormComponent({schema: { type: "string", }, uiSchema});

... which seems to me like it should work but that fails for reasons that currently escape me. I'll need to look at it over the weekend. Got to head off now though!

…dgets,

n1k0

This is now ready to land, thanks for your patience and the great work!

crumblix · 2017-01-20T08:46:40Z

You're welcome :), but I think Textarea widget test is wrong ... isn't it?

n1k0 · 2017-01-20T11:41:27Z

I think Textarea widget test is wrong ... isn't it?

It was, fixed by 8c4707a :)

Breaking changes When a text input is emptied by the user, it's value is now reset to `undefined` instead of being set to `""` (empty string) as previously. This better matches traditional HTML forms behavior. New features * Add an array field template component (#437) * Wrap radio and checkbox labels into span to enable styling. (#428) * Reset text inputs value when emptied (#442) * Add transform hook for changing errors from json schema validation (#432) * Add a noHtml5Validate prop (#448) * Add support for a onBlur event handler (#431) * Allow empty option for enum fields (#451) Bugfixes * Fix #452: Support recursively referenced definitions. (#453) * Fix #454: Document what master actually is, suffix its version with -dev.

n1k0 · 2017-01-28T23:35:44Z

Released in v0.42.0.

EDORAS\mark added 3 commits January 18, 2017 11:27

Enable the required field on arrays

c30f5b4

Update documentation

cc91fa2

Pass empty string text input in as undefined.

8ecafc2

crumblix changed the title ~~Branch6~~ Enable Arrays to be marked as required, and fixes for strings marked as required Jan 18, 2017

This was referenced Jan 18, 2017

Validation - field Email can not be empty if "touched" #402

Closed

Suggestion: in onChange return "undefined" instead of "empty string" or "empty array" #410

Closed

Enable the arrays to be marked as "required" #420

Closed

MoOx reviewed Jan 18, 2017

View reviewed changes

Typos in documentation.

5091525

n1k0 reviewed Jan 18, 2017

View reviewed changes

EDORAS\mark added 5 commits January 18, 2017 20:43

Make code cleaner

8e2ecf1

Make code cleaner

e0fe72e

Make code cleaner

b826fc8

Make code cleaner

2e3c712

Make code cleaner

477d7de

dehli reviewed Jan 18, 2017

View reviewed changes

EDORAS\mark added 2 commits January 20, 2017 08:44

Revert documentation for marking arrays are requried.

ea934ed

Change var to const

fe28e93

EDORAS\mark added 2 commits January 20, 2017 11:17

Revert mandatory changes to arrays.

5f15775

Update documentation to make it clear how to achieve the same result …

7e7a925

…with minItems.

n1k0 reviewed Jan 20, 2017

View reviewed changes

Fix documentation to (hopefully) make clear the distinction between r…

4026366

…equired and minItems properties when used in arrays.

n1k0 reviewed Jan 20, 2017

View reviewed changes

Tests for converting empty string to undefined on Text and TextareaWi…

0e05f7b

…dgets,

n1k0 approved these changes Jan 20, 2017

View reviewed changes

n1k0 merged commit ffc1737 into rjsf-team:master Jan 20, 2017

n1k0 added a commit that referenced this pull request Jan 20, 2017

Refs #442: Fix erroneous test cases.

8c4707a

This was referenced Feb 14, 2017

setting default value for the string input refills the field when emptied up #471

Closed

Refs #471: Introducing field clearing button. #472

Closed

This was referenced Mar 1, 2017

Initial implementation of nullable type #329

Closed

Add support for type: with multiple options #282

Open

spadgett mentioned this pull request Oct 27, 2018

Bug 1605136 - allow empty string values for non-generated parameters openshift/origin#21371

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Arrays to be marked as required, and fixes for strings marked as required #442

Enable Arrays to be marked as required, and fixes for strings marked as required #442

crumblix commented Jan 18, 2017

MoOx Jan 18, 2017

crumblix commented Jan 18, 2017

n1k0 left a comment

n1k0 Jan 18, 2017 •

edited

Loading

crumblix Jan 18, 2017

MoOx Jan 18, 2017

crumblix Jan 18, 2017 •

edited

Loading

olzraiti Jan 18, 2017

n1k0 Jan 18, 2017

dehli Jan 18, 2017

olzraiti Jan 19, 2017

maartenth Jan 19, 2017

n1k0 Jan 19, 2017

n1k0 Jan 18, 2017

n1k0 Jan 18, 2017

n1k0 Jan 18, 2017

n1k0 Jan 18, 2017

n1k0 Jan 18, 2017

crumblix commented Jan 18, 2017

dehli Jan 18, 2017 •

edited

Loading

crumblix commented Jan 19, 2017

crumblix commented Jan 20, 2017

n1k0 left a comment

n1k0 Jan 20, 2017

crumblix Jan 20, 2017

n1k0 Jan 20, 2017 •

edited

Loading

n1k0 Jan 20, 2017

crumblix Jan 20, 2017

crumblix Jan 20, 2017

n1k0 left a comment

crumblix commented Jan 20, 2017

n1k0 commented Jan 20, 2017

n1k0 commented Jan 28, 2017

Enable Arrays to be marked as required, and fixes for strings marked as required #442

Enable Arrays to be marked as required, and fixes for strings marked as required #442

Conversation

crumblix commented Jan 18, 2017

Reasons for making this change

Checklist

Choose a reason for hiding this comment

crumblix commented Jan 18, 2017

n1k0 left a comment

Choose a reason for hiding this comment

n1k0 Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crumblix Jan 18, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crumblix commented Jan 18, 2017

dehli Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

crumblix commented Jan 19, 2017

crumblix commented Jan 20, 2017

n1k0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1k0 Jan 20, 2017 • edited Loading

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

n1k0 left a comment

Choose a reason for hiding this comment

crumblix commented Jan 20, 2017

n1k0 commented Jan 20, 2017

n1k0 commented Jan 28, 2017

n1k0 Jan 18, 2017 •

edited

Loading

crumblix Jan 18, 2017 •

edited

Loading

dehli Jan 18, 2017 •

edited

Loading

n1k0 Jan 20, 2017 •

edited

Loading