Skip to content
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(core): improve Formly config structure #3242

Merged
merged 2 commits into from
Mar 26, 2022

Conversation

aitboudad
Copy link
Member

fix #2853

@netlify
Copy link

netlify bot commented Mar 26, 2022

Deploy Preview for formly-dev ready!

Name Link
🔨 Latest commit 9a29f0c
🔍 Latest deploy log https://app.netlify.com/sites/formly-dev/deploys/623efea9395b440008d7ceb0
😎 Deploy Preview https://deploy-preview-3242--formly-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Mar 26, 2022

Deploy Preview for formly-dev ready!

Name Link
🔨 Latest commit e678517
🔍 Latest deploy log https://app.netlify.com/sites/formly-dev/deploys/623eff9ab2de7f00084750a0
😎 Deploy Preview https://deploy-preview-3242--formly-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Mar 26, 2022

Deploy Preview for formly-dev ready!

Name Link
🔨 Latest commit 78d5197
🔍 Latest deploy log https://app.netlify.com/sites/formly-dev/deploys/623f00fa7cf6e70008aabaec
😎 Deploy Preview https://deploy-preview-3242--formly-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@aitboudad aitboudad merged commit 4df05ce into ngx-formly:main Mar 26, 2022
@aitboudad aitboudad deleted the formly_config branch March 26, 2022 12:10
Copy link
Contributor

@manuelmeister manuelmeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aitboudad I tried to look at some changes.

@@ -56,6 +57,18 @@ export class CoreExtension implements FormlyExtension {
this.getFieldComponentInstance(field).postPopulate?.(field);
}

private initFieldProps(field: FormlyFieldConfigCache) {
if (!field.props) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!field.props) {
field.props ??= field.templateOptions

@@ -34,7 +34,10 @@ export interface FormlyFieldConfig {
/**
* This is reserved for the templates. Any template-specific options go in here. Look at your specific template implementation to know the options required for this.
*/
templateOptions?: FormlyTemplateOptions;
props?: FormlyFieldProps;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export interface FormlyFieldConfig<Props extends FormlyFieldProps> {
  
  props?: Props;

This could be a generic. Then you could make union types of the fields and get better type autocompletion.

E.g:

export interface TextareaFieldConfig extends FormFieldConfig<TextareaProps> {
  type: "textarea";
}

export interface TextareaProps extends FormlyFieldProps {
  row: number;
  cols: number;
}

export type FieldConfigTypes = TextareaFieldConfig | TelephoneFieldConfig | ;

And then I would urgently remove additionalProps as this makes accurate typing much more difficult.

For the transition I would allow these additionalProps only at templateOptions and not at props.

Comment on lines +40 to +41
get props() {
return (this.field.props || {}) as NonNullable<F['props']>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
get props() {
return (this.field.props || {}) as NonNullable<F['props']>;
get props(): NonNullable<F['props']> {
this.field.props ??= {};
return this.field.props;

Is this not needed?

@aitboudad
Copy link
Member Author

@manuelmeister would you like provide a PR for these changes?

@manuelmeister
Copy link
Contributor

I'm currently quite busy, so I'm don't know when I have time to implement this.

@aitboudad
Copy link
Member Author

Ok, will do it then!

aitboudad added a commit to aitboudad/ngx-formly that referenced this pull request May 14, 2022
aitboudad added a commit to aitboudad/ngx-formly that referenced this pull request May 14, 2022
aitboudad added a commit to aitboudad/ngx-formly that referenced this pull request May 14, 2022
aitboudad added a commit to aitboudad/ngx-formly that referenced this pull request May 21, 2022
aitboudad added a commit to aitboudad/ngx-formly that referenced this pull request May 21, 2022
aitboudad added a commit to aitboudad/ngx-formly that referenced this pull request May 21, 2022
aitboudad added a commit to aitboudad/ngx-formly that referenced this pull request May 21, 2022
aitboudad added a commit that referenced this pull request May 21, 2022
@aitboudad aitboudad mentioned this pull request Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Improve Formly config structure
2 participants