-
Notifications
You must be signed in to change notification settings - Fork 395
Mixed support #2409
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
base: master
Are you sure you want to change the base?
Mixed support #2409
Conversation
kchobantonov
commented
Dec 30, 2024
•
edited
Loading
edited
- provide support for mixed types
- fix clearing of inputs when not under an object - e.g. string once cleared will become empty string rather than to return undefined to remove the property from the parent object
- fix date controls, now the date can show year, year/month and etc. depending on the control option views
- improve the performance to convert dayjs format into maska format function
- fixing the :key to at least have the collection size into the key so when the size changes the vue will not reuse html elements leaving attributes like id unchanged
- add more examples to show how the mixed control can be used
- use arraySchema from the mapped values rather than using Resolve
- minor UI changes to use less space
- core changes : use the provided schema when the resolve failed, make sure that boolean is valid schema as well - e.g. items: true is valid schema (Note: the last change for the items to be boolean was reverted back since more code should be adjusted - maybe a future improvement)
- introduce a new config - allowAdditionalPropertiesIfMissing - this allows to show the additioanalProperties UI when the additionalProperties and patternProperties do not exist. The reason is that by default if the additionalProperties is missing from the schema the default should be considered as true but the UI component does not respect that default - e.g. it needs to be explicitly states as additionalProperties: true if the UI control is needed without the allowAdditionalPropertiesIfMissing config
…ot part of the object that has the property defined in properties then to not clear it - e.g. send undefined value
… based on index - e.g. prevent reusing elements when the array change size which will cause id properties for example to not have correct value
…tified for the new value
…n also allow such case to be handled by the object renderer
…s no need to resolve that schema just use it
…n the mixed renderer selected object type
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
… in the core should be adjusted to handled that case
@sdirix please review - checkout the new examples
For example 3 the easier way to see that is to copy any example json data, provided it in the example ( Data tab - save it) and see the result in the Demo tab |
Thanks for the contribution ❤️ I'll take a look within January 👍 |
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.
Hi @kchobantonov,
I had a first look at the implementation. The examples work great and are really impressive ❤️
I added some comments, please have a look
if ( | ||
typeof schema === 'boolean' && | ||
(!pathSegments || pathSegments.length === 0) | ||
) { | ||
// add the case where the schema can be true value for example: items: true or additionalProperties: false | ||
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.
What is the use case of this? Who hands over segments to resolve if the schema is just a boolean already anyway?
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.
that is to resolve the case where the value of the last path segment is equals to true. For example items: true and you want to bind that items then you need to resolve the jsonschema which can be boolean but in the code we ignore that this can be ever the case.
Also the next check for isEmpty if we pass true/false values which are valid JSON schema will return an undefined when resolved instead of boolean
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.
The case is already handled via
if (!pathSegments || pathSegments.length === 0) {
return schema;
}
If having a boolean schema
breaks the checks before that, then we should fix those checks instead of hard coding the exception at the top.
I guess the typing does not properly reflect that the schema
could be a boolean. So we should adjust the typing and the checks to handle the typing.
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.
the issue was that if I was going to adjust the typing to allow true/false to be valid schema then there was a lot more changes to the code that just this one - so if the goal is go make the typing correct and the amount of changes to not be an issue then that is fine but I didn't wanted to change a lot of stuff in the core packages that was the reason to have that there
@@ -20,7 +20,7 @@ | |||
/> | |||
<dispatch-renderer | |||
v-for="(allOfRenderInfo, allOfIndex) in allOfRenderInfos" | |||
:key="`${control.path}-${allOfIndex}`" | |||
:key="`${control.path}-${allOfRenderInfos.length}-${allOfIndex}`" |
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.
Why adapt this key?
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.
because if the size of the allOfRendererInfo changes then the key is not stable enough to guarantee that previous keys will point the exact same elements - e.g. we do not have a primary key for the info that we can use - usually this happens when we change the UI schema and the schema have to be applied again.
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.
Seems weird as this only fixes the case in which the uischema
or schema
changes and the length ALSO changes. It will not work if the length does not change.
I wonder whether there should be a change higher up, for example setting a key on the whole form and increasing it whenever we hand over a new schema
or uischema
. This would be a more generic fix covering all use cases, instead of only a partial fix, covering some of the use cases.
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 agree as well but then we do need that on the core level - for now at least that is fixing the issue that I was having when I change the uischema from the monaco editor in our vue examples. So once this is available at the core - maybe some id for the uischema changes then we can utilize that.
packages/vue-vuetify/src/extended/AutocompleteEnumControlRenderer.vue
Outdated
Show resolved
Hide resolved
packages/vue-vuetify/src/extended/AutocompleteOneOfEnumControlRenderer.vue
Outdated
Show resolved
Hide resolved
packages/vue-vuetify/src/controls/AnyOfStringOrEnumControlRenderer.vue
Outdated
Show resolved
Hide resolved
@sdirix check my responses and also updated repo but it looks like the build failed perhaps because netlify is using greater than pnpm version 8 now ? |
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 have concerns about the proposed changes to the core functionality.
Please have a look at my new comments and the unresolved comments from my previous review.
if ( | ||
typeof schema === 'boolean' && | ||
(!pathSegments || pathSegments.length === 0) | ||
) { | ||
// add the case where the schema can be true value for example: items: true or additionalProperties: false | ||
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.
The case is already handled via
if (!pathSegments || pathSegments.length === 0) {
return schema;
}
If having a boolean schema
breaks the checks before that, then we should fix those checks instead of hard coding the exception at the top.
I guess the typing does not properly reflect that the schema
could be a boolean. So we should adjust the typing and the checks to handle the typing.
@@ -20,7 +20,7 @@ | |||
/> | |||
<dispatch-renderer | |||
v-for="(allOfRenderInfo, allOfIndex) in allOfRenderInfos" | |||
:key="`${control.path}-${allOfIndex}`" | |||
:key="`${control.path}-${allOfRenderInfos.length}-${allOfIndex}`" |
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.
Seems weird as this only fixes the case in which the uischema
or schema
changes and the length ALSO changes. It will not work if the length does not change.
I wonder whether there should be a change higher up, for example setting a key on the whole form and increasing it whenever we hand over a new schema
or uischema
. This would be a more generic fix covering all use cases, instead of only a partial fix, covering some of the use cases.
@sdirix please review again - here are the outstanding items that I need to check with you if you are ok with those as they are at the moment with the explanation that was provided.
|