-
Notifications
You must be signed in to change notification settings - Fork 373
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
Provide additional tester context #1975
Provide additional tester context #1975
Conversation
core: * Change the third parameter of Tester and RankedTester from just the root schema to a new TesterContext object. It contains the root schema and the global config. * Remove UI Schema generation from mapStateToJsonFormsRendererProps. Instead, it is taken from the state. react: * Rename ctxToJsonFormsDispatchProps to ctxToJsonFormsRendererProps * Add HOC withJsonFormsRendererProps that injects state props vue: * Upgrade @vue/test-utils to the latest stable version * Add config as an optional renderer prop Fixes eclipsesource#1970
3f9b74d
to
fd93428
Compare
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.
Works great. Just very minor comments.
const testerContext = useMemo( | ||
() => ({ | ||
rootSchema: rootSchema, | ||
config: config | ||
}), | ||
[rootSchema, config] | ||
); | ||
const cell = useMemo( | ||
() => maxBy(cells, r => r.tester(uischema, schema, testerContext)), | ||
[cells, uischema, schema, testerContext] | ||
); | ||
if (cell === undefined || cell.tester(uischema, schema, testerContext) === -1) { |
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.
Personally I would have done one useMemo
as the standalone testerContext
useMemo
is not really necessary but that's just a minor issue.
const testerContext = useMemo( | ||
() => ({ | ||
rootSchema: props.rootSchema, | ||
config: props.config | ||
}), | ||
[props.rootSchema, props.config] | ||
); | ||
const renderer = useMemo( | ||
() => maxBy(props.renderers, r => r.tester(props.uischema, props.schema, props.rootSchema)), | ||
[props.renderers, props.uischema, props.schema] | ||
() => maxBy(props.renderers, r => r.tester(props.uischema, props.schema, testerContext)), | ||
[props.renderers, props.uischema, props.schema, testerContext] | ||
); | ||
if ( | ||
renderer === undefined || | ||
renderer.tester(props.uischema, props.schema, props.rootSchema) === -1 | ||
renderer.tester(props.uischema, props.schema, testerContext) === -1 | ||
) { |
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.
same here
MIGRATION.md
Outdated
Therefore the manual resolving of JSON Schemas before handing them over to JSON Forms does not need to be performed in those cases. | ||
In addition, testers can now access the global `config` to consider default UI Schema options. |
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.
There are also other use cases so I would just add that the config
is now available. Especially as we don't use the config in this PR.
MIGRATION.md
Outdated
Therefore the manual resolving of JSON Schemas before handing them over to JSON Forms does not need to be performed in those cases. | ||
In addition, testers can now access the global `config` to consider default UI Schema options. |
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.
Instead of global
I usually use the term "form wide" to avoid confusion
packages/core/src/testers/testers.ts
Outdated
export interface TesterContext { | ||
/** The root JsonSchema of the form. Can be used to resolve references. */ | ||
rootSchema: JsonSchema; | ||
/** The global configuration object given to JsonForms. Can be used to derive default UISchema options. */ |
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.
same here
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.
LGTM
core:
react:
vue:
Fixes #1970