added additional support #458

riba2101 · 2017-02-02T11:21:57Z

Reasons for making this change

added additional support #228
If this is related to existing tickets, include links to them as well.

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

just use a scheme that contains the key

Schema:

{
	"title": "A registration form",
	"description": "A simple form example.",
	"$schema": "http://json-schema.org/draft-04/schema#",
	"type": "object",
	"properties": {
		"name": {
			"type": "string"
		},
		"devMap": {
			"type": "object",
			"additionalProperties": {
				"type": "integer"
			}
		}
	}
}

formData:

{
	"name": "test",
	"devMap": {
		"key": 2,
		"value": 12
	}
}

stathismor · 2017-02-02T11:36:29Z

@riba2101 Nice to see progress on this!
Does this support uiShema?
Also you've ticked that you have updated the playground, but I don't see in the code that you have...

riba2101 · 2017-02-02T11:53:44Z

uiSchema support is somewhat basic, would be grate to extend it but given my "no time atm to play"...
added to playgorund(i've pasted the example here in the PR)

glasserc

Thanks for your effort! This is a good start, but it's still quite limited. I noticed, for instance, that it's not possible to add more properties to e.g. devMap, which seems like quite a natural feature to want. And of course, it doesn't support stranger cases like additionalProperties: {}, which may be too hard/out of scope for this project.

glasserc · 2017-02-21T19:10:27Z

.gitignore

@@ -4,3 +4,5 @@ build
 dist
 lib

+/*.iml
+.idea


Please don't commit these to the project's .gitignore. If you find them helpful, you might add them to your $HOME/.config/git/ignore (per the gitignore man page).

glasserc · 2017-02-21T19:20:52Z

playground/samples/additionalProperties.js

+ "name": "test",
+ "devMap": {
+ "key": 2,
+ "value": 12


I think this example is quite confusing. additionalProperties makes the most sense for properties with unknown and arbitrary names. The names key and value sound like two very specific names with a predefined meaning, and specifically they sound like an attempt to try to encode some entity like {"5": 12}. ({5: 12} is, of course, invalid JSON, since all keys have to be strings.) I think this is especially misleading because of the name devMap.

How about an example where the keys are really arbitrary, for example site names or URLs? I guess we'd still want the values to be numeric, so maybe it could be a map of site -> estimated user count or something?

glasserc · 2017-02-21T19:21:14Z

src/components/fields/ObjectField.js

- return true;
- }
- return false;
+ return !deepEquals(newKeys.sort(), oldKeys.sort());


Please do not commit stylistic changes as part of this PR, it makes it harder to review.

glasserc · 2017-02-21T19:22:30Z

src/utils.js

@@ -112,7 +112,7 @@ function computeDefaults(schema, parentDefaults, definitions={}) {
 if (isObject(defaults) && isObject(schema.default)) {
 // For object defaults, only override parent defaults that are defined in
 // schema.default.
- defaults = mergeObjects(defaults, schema.default);
+ defaults = merge(defaults, schema.default);


Switching from the existing mergeObjects function to lodash.merge is its own topic for discussion and should not be part of this PR. It makes it seem like using lodash.merge is necessary to implement this feature, which as far as I can tell, it isn't.

glasserc · 2017-02-21T19:34:19Z

src/components/fields/ObjectField.js

+  </p>
+  <pre>{JSON.stringify(schema)}</pre>
+  </div>
+  );


Please do not change indentation, here as well as elsewhere, as it no longer matches the style of the rest of the code, and the changes themselves are hard to review. I see that you unindented the code that eslint flagged, so clearly our eslint rules aren't strict enough about enforcing "house style", but please try to match it anyhow :)

glasserc · 2017-02-21T19:41:23Z

src/components/fields/ObjectField.js

 const schema = retrieveSchema(this.props.schema, definitions);
 const title = (schema.title === undefined) ? name : schema.title;
+ if ("additionalProperties" in schema){


Nit: Space before {

glasserc · 2017-02-21T19:57:45Z

src/components/fields/ObjectField.js

+ );
+ })}
+ </fieldset>
+ );


I don't think it's necessary to copy and paste this entire function. There are more commonalities than differences in the two implementations, which means most parts are unchanged. Trying to review this is challenging because I have to read a whole new method rather than an enhanced version of the existing method.

glasserc · 2017-02-21T19:59:24Z

src/components/fields/ObjectField.js

+ id={`${idSchema.$id}__description`}
+ description={schema.description}
+ formContext={formContext}/> : null}
+ {Object.keys(this.state).map((name, index) => {


Of the ways that this is different from the existing method, I don't understand why we no longer respect ui:order here.

glasserc · 2017-02-21T20:00:31Z

src/components/fields/ObjectField.js

+ <SchemaField key={index}
+ name={name}
+ required={this.isRequired(name)}
+ schema={schema.additionalProperties}


I'm pretty sure this doesn't correctly support a schema that has both properties and additionalProperties.

glasserc · 2017-02-21T20:05:20Z

src/utils.js

@@ -129,6 +129,10 @@ function computeDefaults(schema, parentDefaults, definitions={}) {
 }
 // We need to recur for object schema inner default values.
 if (schema.type === "object") {
+ if (!schema.properties){
+ return defaults;
+ }


What is this change about?

atxpace · 2017-05-24T19:48:20Z

What is the status of this. Is it being actively worked on?

riba2101 · 2017-05-25T08:11:58Z

Hey, sry but I'm currently seriously lacking time. Feel free to take it over

epicfaace · 2019-01-20T19:35:05Z

Closing this PR as additionalProperties support was added in #1021.

riba2101 added 3 commits February 2, 2017 12:17

added additional support

5c2e56e

cleanup

d1f5f18

fix

19520bf

samples

71363a8

glasserc requested changes Feb 21, 2017

View reviewed changes

epicfaace closed this Jan 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added additional support #458

added additional support #458

riba2101 commented Feb 2, 2017 •

edited by n1k0

Loading

stathismor commented Feb 2, 2017 •

edited

Loading

riba2101 commented Feb 2, 2017

glasserc left a comment

glasserc Feb 21, 2017

glasserc Feb 21, 2017

glasserc Feb 21, 2017

glasserc Feb 21, 2017

glasserc Feb 21, 2017

glasserc Feb 21, 2017

glasserc Feb 21, 2017

glasserc Feb 21, 2017

glasserc Feb 21, 2017

glasserc Feb 21, 2017

atxpace commented May 24, 2017

riba2101 commented May 25, 2017

epicfaace commented Jan 20, 2019

@@ @@ -4,3 +4,5 @@ build @@
 dist
 lib
+/*.iml
+.idea

added additional support #458

added additional support #458

Conversation

riba2101 commented Feb 2, 2017 • edited by n1k0 Loading

Reasons for making this change

Checklist

stathismor commented Feb 2, 2017 • edited Loading

riba2101 commented Feb 2, 2017

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

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

atxpace commented May 24, 2017

riba2101 commented May 25, 2017

epicfaace commented Jan 20, 2019

riba2101 commented Feb 2, 2017 •

edited by n1k0

Loading

stathismor commented Feb 2, 2017 •

edited

Loading