-
Notifications
You must be signed in to change notification settings - Fork 379
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
feat: added select control renderer for enums and oneOf #2264
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hi! Thanks for your PR ❤️ We'll try to have a look soon! |
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 @mhorsche , thanks again for the contribution ❤️
The control already seems to work quite well. I have some inline comments.
Please have a look at the comments and add some test cases for the new renderer in packages/angular-material/test
. Generally, we require at least basic tests for every renderer.
/* | ||
The MIT License | ||
|
||
Copyright (c) 2017-2019 EclipseSource Munich |
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.
Copyright (c) 2017-2019 EclipseSource Munich | |
Copyright (c) 2024 EclipseSource Munich |
return { | ||
const: el.const, | ||
label: el.title ? el.title : el.const, | ||
// disabled: el.readOnly ? true : 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.
Can this be removed?
<mat-option | ||
*ngFor="let option of optionElements" | ||
[value]="option.const" | ||
[disabled]="option.disabled ? true : 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.
Can this be removed?
) | ||
); | ||
|
||
interface OptionOfSelect { |
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 defining a new type, I think we can reuse the EnumOption
type from @jsonforms/core
. The disabled
property does not seem to be needed after all.
/* change scope to items of the array */ | ||
scope = scope.items as JsonSchema; | ||
} | ||
console.log(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.
Is this a leftover from development? It should likely be removed
|
||
export const SelectControlRendererTester: RankedTester = rankWith( | ||
5, | ||
/* Can be used for simple Enums or OneOf, autocomplete functionallity needs autocomplete.rederer */ |
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.
/* Can be used for simple Enums or OneOf, autocomplete functionallity needs autocomplete.rederer */ | |
/* Can be used for simple Enums or OneOf, autocomplete functionality needs autocomplete.renderer */ |
Added
<mat-select>
control for enum and oneOf types. Enable multi-selection for array type. Not sure if RankedTester ist properly defined, could need help with that.