-
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
If/Then/Else - Code Review changes from #2506 and tests from #2466 #2700
If/Then/Else - Code Review changes from #2506 and tests from #2466 #2700
Conversation
Added if then else logic to resolve schemas
packages/core/src/utils.js
Outdated
if (conditionalSchema) { | ||
return retrieveSchema( | ||
mergeSchemas( | ||
retrieveSchema(conditionalSchema, rootSchema, formData), |
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.
{
if: ...,
then: {minLength: 5},
minLength: 10
}
Expected behavior: inner property (minLength: 5
) should override outer property (minLength: 10
). We should add a test case for this
And potentially swap these two arguments.
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 test case correctly captures the issue 🎉
packages/core/src/utils.js
Outdated
export function resolveSchema(schema, rootSchema = {}, formData = {}) { | ||
if (schema.hasOwnProperty("$ref")) { | ||
return resolveReference(schema, rootSchema, formData); | ||
} else if (schema.hasOwnProperty("dependencies")) { | ||
const resolvedSchema = resolveDependencies(schema, rootSchema, formData); | ||
return retrieveSchema(resolvedSchema, rootSchema, formData); | ||
} else if (schema.hasOwnProperty("if")) { |
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.
See if we can move this to retrieveSchema instead and everything still works.
return retrieveSchema(resolvedSchemaLessConditional, rootSchema, formData); | ||
} | ||
}; | ||
|
||
export function resolveSchema(schema, rootSchema = {}, formData = {}) { |
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.
Document this
// Resolves references and dependencies and references within branches of allOf
// Called internally by retrieveSchema
expect(node.querySelector("input[label=postal_code]")).not.eql(null); | ||
}); | ||
|
||
it("should should render control when data has not been filled in", () => { |
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("should should render control when data has not been filled in", () => { | |
it("should render control when data has not been filled in", () => { |
formData, | ||
}); | ||
|
||
// This feels backwards but is basically because undefined equates to true when field is validated |
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 feels backwards but is basically because undefined equates to true when field is validated | |
// An empty formData will make the conditional evaluate to true because no properties are required in the if statement |
…schema-form into 2506-code-review-changes
Estimate on when this will be released? @epicfaace |
Reasons for making this change
I added changes from code review to the PR in #2506 and added integration tests from my PR in #2466
Checklist