Handle fixed items arrays #132

rhgb · 2016-04-11T06:21:27Z

fixed #76
handle fixed items arrays:

{
  "type": "array",
  "title": "List of fixed items",
  "items": [
    {
      "type": "string",
      "title": "Some text"
    },
    {
      "type": "number",
      "title": "A number"
    }
  ]
}

NOTE: According to the spec, it should be valid that form data has less items in fixed array than the schema defined, but it seems hard to find an appropriate UI for this. I'm glad to implement it if there's any reasonable suggestion.

n1k0 · 2016-04-11T13:58:25Z

src/utils.js

+
+export function allowAdditionalItems(schema) {
+  return isObject(schema.additionalItems);
+}


As spec says that "additionalItems" MUST be either a boolean or an object.

We should therefore fallback on the the default items schema if allowAdditionalItems is true.

In the spec it says:

8.2.3.1. If items is a schema, then the child instance must be valid against this schema, regardless of its index, and regardless of the value of "additionalItems".

Therefore it is only valid when items is an array, and when additionalItems is true it implies any value of additional items is valid. IMO falling back to items or items[0] are not best choices. (Maybe we need a input widget for "any" or "raw"?)

Maybe we need a input widget for "any" or "raw"?

Well, that's quite impossible, even if we decide to render a textarea it's unlikely to be able to reflect any JSON data structure, so we're stuck here.

Maybe we should simply raise a warning in such a case, and document this limitation in an appropriate section of the documentation? What do you think?

You are right, maybe that's the best way currently. For the detail should we display a message in the error list, or just showing a warning in the console?

For the detail should we display a message in the error list, or just showing a warning in the console?

A warning in the console sounds good enough to me. I'd like to keep form error reporting for strict jsonschema validation stuff, not lib limitations.

n1k0 · 2016-04-11T14:14:57Z

This looks great with a few comments to address. Thanks a lot for contributing.

n1k0 · 2016-04-11T16:04:33Z

BTW I landed a fix for #135 so the playground now works again and allows to run your added examples. You should probably rebase this PR on top of master to benefit from it.

rhgb · 2016-04-12T01:55:57Z

Thanks for the comment: )

rhgb · 2016-04-14T05:30:07Z

Fixed issues above. For the case additionalItems: true, a warning message will be printed to console, and some explanations are added to README.md

n1k0 · 2016-04-14T08:41:46Z

src/components/fields/ArrayField.js

+            const itemIdPrefix = idSchema.id + "_" + index;
+            const itemIdSchema = toIdSchema(itemsSchema, itemIdPrefix, definitions);
+            return this.renderArrayFieldItem({
+              index, itemSchema: itemsSchema, itemIdSchema, itemErrorSchema,


Nit: Each key assignment on its own line please.

n1k0 · 2016-04-14T08:46:31Z

Looks great. Do you think it could be possible to extract rendered JSX from comp methods into their own separate stateless components?

rhgb · 2016-04-14T09:15:45Z

I'll have a try. But it seems these components will have 7 or 8 props to be passed in. I'm not sure whether it is too many for a component. What's your suggestion?

n1k0 · 2016-04-14T11:21:24Z

But it seems these components will have 7 or 8 props to be passed in

I see what you mean, this could easily turn verbose and messy. Plus component methods are certainly convenient as they have natural immediate access to the prototype.

I tend to agree with the current approach, even if it bloats a little the whole ArrayField component; though that's expected as we're now covering a lot of use cases. We can always refactor later on if we keep adding stuff anyway, so that's definitely not a blocker here.

I'm going to do a second pass over the diff, but I have a feeling we are very close from having our final patch.

n1k0 · 2016-04-14T11:41:46Z

README.md

@@ -636,6 +636,13 @@ This library partially supports [inline schema definition dereferencing]( http:/

 Note that it only supports local definition referencing, we do not plan on fetching foreign schemas over HTTP anytime soon. Basically, you can only reference a definition from the very schema object defining it.

+## JSON Schema supporting status


You should run the npm run build:readme so this entry will be added to the TOC.

n1k0 · 2016-04-14T13:40:09Z

this whole approach is vulnerable to carefully picked names

Personally I can live with it, but I wonder if you had a better or more solid parsing strategy in mind?

Also, it seems we should add some unit tests to cover the new cases handled by this last change.

rhgb · 2016-04-14T14:40:07Z

I'll add some test for that.

In my opinion maybe we should let components maintain their own error status. I'm not very experienced in react so I'd like to hear your suggestion. Also there seems to be a lot of work, maybe we discuss it later in another thread?

n1k0 · 2016-04-14T15:43:37Z

In my opinion maybe we should let components maintain their own error status

My initial idea was to prevent field components to deal with state at all; of course it's not possible for arrays and objects, as they both carry one while the user is "building" them -- which kinda defeats the purpose admittedly. For errors, validating at the field level may get harder are you often need the parent schema eg. for required or unique fields, but that's just plain theory from me.

Feel free to file an issue if you think there's a better way :)

n1k0 · 2016-04-15T13:10:14Z

@rhgb Sorry I had to land #137 and issue a v0.24.0 asap, so your patch is now conflicting. Just tell me if you prefer me to take over the branch if you don't have the time to rebase it on top of latest master, I'll happily do that.

rhgb · 2016-04-16T07:03:38Z

It's fine, I'll rebase it myself and add tests we've discussed before. Just have too much other work to do yesterday, didn't finish those tests yet.

fix typo & code style

rhgb · 2016-04-18T02:51:54Z

I've done rebasing master and modified some test to fit new changes of #139. Tests for error string parsing also added.

n1k0 · 2016-04-18T06:42:35Z

Fantastic work, thanks a lot for your work!

n1k0 reviewed Apr 11, 2016
View reviewed changes

rhgb force-pushed the array-tuple branch from 62abc24 to fdbaa34 Compare April 14, 2016 05:22

n1k0 reviewed Apr 14, 2016
View reviewed changes

n1k0 mentioned this pull request Apr 14, 2016

Fixes #137: Prevent non-required fields to fill state with defaults. #139

Merged

rhgb added 16 commits April 18, 2016 10:34

Implement fixed item arrays

f1d51b0

add tests

d31e429

support "additionalItems"

9d279d7

add test for additionalItems

2190314

minor fixes

454f1e4

add test

4674988

restructure ArrayField.js

348729c

update readme

a30e635

uischema for fixed items

0daa9ad

linting

0e37087

restructured renderFixedArray()

4c9610b

fix typo & code style

fix indent

f4dfad5

some comments

fe3c3a7

indent

466642c

fix bug

54127e6

modify tests to meet rjsf-team#139

75215bf

rhgb force-pushed the array-tuple branch from 704bcba to 2a535e3 Compare April 18, 2016 02:46

test for errors of nested arrays

135cd8e

rhgb force-pushed the array-tuple branch from 2a535e3 to 135cd8e Compare April 18, 2016 02:48

n1k0 merged commit 22962b2 into rjsf-team:master Apr 18, 2016

rhgb deleted the array-tuple branch April 19, 2016 04:51

n1k0 mentioned this pull request Apr 19, 2016

Custom field validation #145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle fixed items arrays #132

Handle fixed items arrays #132

rhgb commented Apr 11, 2016

n1k0 Apr 11, 2016

rhgb Apr 12, 2016

n1k0 Apr 12, 2016

rhgb Apr 13, 2016

n1k0 Apr 13, 2016

n1k0 commented Apr 11, 2016

n1k0 commented Apr 11, 2016

rhgb commented Apr 12, 2016

rhgb commented Apr 14, 2016

n1k0 Apr 14, 2016

n1k0 commented Apr 14, 2016

rhgb commented Apr 14, 2016

n1k0 commented Apr 14, 2016

n1k0 Apr 14, 2016

n1k0 commented Apr 14, 2016

rhgb commented Apr 14, 2016

n1k0 commented Apr 14, 2016

n1k0 commented Apr 15, 2016 •

edited

Loading

rhgb commented Apr 16, 2016

rhgb commented Apr 18, 2016

n1k0 commented Apr 18, 2016

		@@ -636,6 +636,13 @@ This library partially supports [inline schema definition dereferencing]( http:/

		Note that it only supports local definition referencing, we do not plan on fetching foreign schemas over HTTP anytime soon. Basically, you can only reference a definition from the very schema object defining it.

		## JSON Schema supporting status

Handle fixed items arrays #132

Handle fixed items arrays #132

Conversation

rhgb commented Apr 11, 2016

n1k0 Apr 11, 2016

Choose a reason for hiding this comment

rhgb Apr 12, 2016

Choose a reason for hiding this comment

n1k0 Apr 12, 2016

Choose a reason for hiding this comment

rhgb Apr 13, 2016

Choose a reason for hiding this comment

n1k0 Apr 13, 2016

Choose a reason for hiding this comment

n1k0 commented Apr 11, 2016

n1k0 commented Apr 11, 2016

rhgb commented Apr 12, 2016

rhgb commented Apr 14, 2016

n1k0 Apr 14, 2016

Choose a reason for hiding this comment

n1k0 commented Apr 14, 2016

rhgb commented Apr 14, 2016

n1k0 commented Apr 14, 2016

n1k0 Apr 14, 2016

Choose a reason for hiding this comment

n1k0 commented Apr 14, 2016

rhgb commented Apr 14, 2016

n1k0 commented Apr 14, 2016

n1k0 commented Apr 15, 2016 • edited Loading

rhgb commented Apr 16, 2016

rhgb commented Apr 18, 2016

n1k0 commented Apr 18, 2016

n1k0 commented Apr 15, 2016 •

edited

Loading