-
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
Add preliminary support for object "additionalProperties" #1021
Conversation
src/components/fields/ObjectField.js
Outdated
if (!schema.additionalProperties) { | ||
return false; | ||
} | ||
let { expandable } = getUiOptions(uiSchema); |
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.
No need to make expandable a let:
const { expandable } = getUiOptions(uiSchema);
if (expandable !== false) {
// if ui:options.expandable was not explicitly set to false, we can add
// another property if we have not exceeded maxProperties yet
if (schema.maxProperties !== undefined) {
return Object.keys(formData).length < schema.maxProperties;
} else {
return true;
}
}
return expandable;
src/components/fields/ObjectField.js
Outdated
@@ -42,6 +69,13 @@ class ObjectField extends Component { | |||
readonly: false, | |||
}; | |||
|
|||
constructor(props) { | |||
super(props); | |||
this.state = { |
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.
it looks like we've got the class fields proposal enabled, so you can do this with:
state = {
additionalProperties: {},
}
src/components/fields/ObjectField.js
Outdated
} | ||
} | ||
|
||
handleAddClick = schema => { |
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.
handleAddClick = schema => () = >
src/utils.js
Outdated
} | ||
return schema; |
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.
avoid reassigning params
* avoid modifying parameters * avoid unnecessary let using returns * eliminate unnecessary bind * terse closure return syntax * take advantage of class fields
a7b39d0
to
cd61fe8
Compare
Thanks for the great feedback @graingert! |
🙏 👍 |
@christianclent CI failed because of the timeout.. I think all you have to do is to rerun it ;) It would be a big loss if such a remarkable work couldnt be merged 👍 @graingert could you rerun ?:) |
… additionalProperties rjsf-team#1021'
@christianclent maybe close this and open new PR as this will trigger travis again https://stackoverflow.com/questions/17606874/trigger-a-travis-ci-rebuild-without-pushing-a-commit |
Thank you for the hot tip @derberg! I've been using this branch in one of my personal projects for a few weeks now, and there are definitely areas I would like to improve.
Some of those may be feasible to fix in the next few weekends if they are deal-breakers for anybody. |
Hi @christianclent, @graingert Could you please take another look at this PR? Thanks! |
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.
This looks pretty good! One feature of rjsf is that traditionally we preserve properties that aren't described by the schema (although we don't show them in any way). Is that still true here, in the cases where additionalProperties
is false?
My other concern is about UX. Each property is two labeled inputs, and it isn't obvious at first glance that they're related. Is there any way we could use the first input as the label to the second input? Or is that just crazy talk?
src/components/fields/ObjectField.js
Outdated
} | ||
return true; | ||
} | ||
return expandable; |
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.
You could refactor this as
if (expandable === false) {
return false;
}
if (schema.maxProperties !== undefined) {
return Object.keys(formData).length < schema.maxProperties;
}
return true;
This would save a level of indentation. But that's just a personal preference.
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.
I think this is a widely held preference, and I strongly agree. Some folks call this the "bouncer pattern": http://rikschennink.nl/thoughts/the-bouncer-pattern/
src/components/fields/ObjectField.js
Outdated
errorSchema && | ||
this.props.errorSchema && { | ||
...this.props.errorSchema, | ||
[name]: errorSchema, |
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.
Should name
here be value
? Or more generally, where does name
come from in this scope?
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.
Haha, yes, this was copy pasta from onPropertyChange
src/utils.js
Outdated
const resolvedSchema = resolveSchema(schema, definitions, formData); | ||
const hasAdditionalProperties = | ||
resolvedSchema.hasOwnProperty("additionalProperties") && | ||
resolvedSchema.hasAdditionalProperties !== false; |
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.
What is the hasAdditionalProperties
property? Is this maybe supposed to be additionalProperties
?
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.
Yes indeed!
Great points @glasserc! I didn't even consider preserving non-schema properties. It would likely not be difficult to. The following is my instinct for ideal behavior, but I am very open to whatever folks like best. Whatever we decide, this certainly calls for unit tests! Yes, that is a very good idea - I considered something like it, but ultimately chose to prioritize power and flexibility over user experience. In hindsight, I think you are right and UX should be paramount. Another slightly different idea is to make the key related to the title (perhaps via something like suglify: https://www.npmjs.com/package/slugify ). I will address these points and your specific feedback by EOD today with any luck. |
Fix misnamed "additionalProperties" attribute Invert conditional to reduce indentation Added unit tests to ensure non-schema properties are maintained
It turns out that this continues to preserve non-schema properties in every case. That being said, the existing validation should complain if the user sets non-schema properties and sets additionalProperties to false. I have added unit tests for this behavior. I haven't changed the user experience for keys yet. Do you definitely like using the title as the object key? If so, I'll try to get to that this week. |
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.
This is great, thanks! It's clearly an improvement over what we have now, so I'll merge it. I'll add a note to the documentation that the UX is still experimental for now, and maybe someone else, or even you, will revisit it.
@glasserc Thank you for merging this! Do you plan a new release soon? |
Reasons for making this change
This will add the ability for the form to understand "additionalProperties" definitions in the schema (see json-schema properties documentation).
Related issue: #228
Checklist
npm run cs-format
on my branch to conform my code to prettier coding styleDetails
Specifically, this will enable the following behaviors when additionalProperties is enabled:
Opportunities for improvement: