-
Notifications
You must be signed in to change notification settings - Fork 2
CMS-46358 Add property group support #153
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
Conversation
…t property groups
… and remove unused functions
exacs
left a comment
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 good overall, but I have concerns with typing
| required?: boolean; | ||
| localized?: boolean; | ||
| group?: string; | ||
| group?: PropertyGroupKey; |
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.
Does this mean that I have to declare the types (i.e. add to the PropertyGroupRegistry interface) for this to work?
What happens if I have define a property group in the CMS via UI?
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 change was introduced to allow built in types with string (I'll double check if strings are accepted). But If a developer defines a propertyGroup in CMS it wont show up as autocomplete or type-safe. If a developer use module augmentation using PropertyGroupRegistry interface it will show up as auto complete.
…y and refactor code.
propertyGroupsin config file.propertyGroupsin CLI.propertyGroups.Note: type-safety is not implemented in this PR due to the lack of typescript support and
.mjsin config file which limits us on using module augmentation. More info can be found in CMS-46487