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

#230 added methods to extract nested properties & create empty objects #293

Merged
merged 4 commits into from
Nov 29, 2016
Merged

Conversation

Krustie101
Copy link
Contributor

@Krustie101 Krustie101 commented Nov 26, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix. In formly form, the fieldModel method assumes that the key is not nested. This bugfix also deals with the case that the key is a nested property (including support for indices, if you really want to use this). The main code is located in the utils class.

Tests have been added for the extra code in the utils class.

What is the current behavior? (You can also link to an open issue here)
See #230

What is the new behavior (if this is a feature change)?

Please check if the PR fulfills these requirements

Please provide a screenshot of this feature before and after your code changes, if applicable.

Other information:

@mohammedzamakhan
Copy link
Contributor

@Krustie101 it worked for me when I did

fieldModel(field: FormlyFieldConfig) {
     if (field.key && (field.fieldGroup || field.fieldArray)) {
       return getFieldModel(this.model, field, true);
     }
     return this.model;
}

@Krustie101
Copy link
Contributor Author

@mohammedzamakhan That is correct. I also noticed my error, arising from the fact that I did not pay attention that you had changed the meaning of model.

Conflicts:
	src/core/components/formly.form.ts
	src/core/utils.spec.ts
@codecov-io
Copy link

Current coverage is 92.62% (diff: 97.36%)

Merging #293 into master will increase coverage by 1.99%

@@             master       #293   diff @@
==========================================
  Files            16         16          
  Lines           672        705    +33   
  Methods         140        143     +3   
  Messages          0          0          
  Branches        163        174    +11   
==========================================
+ Hits            609        653    +44   
+ Misses           63         52    -11   
  Partials          0          0          

Powered by Codecov. Last update fb2fc94...105197b

@mohammedzamakhan
Copy link
Contributor

LGTM 👍

@aitboudad can you have a look at this PR and approve it?

@aitboudad
Copy link
Member

@mohammedzamakhan I can't right now :( you can merge it if it looks good,
I'll give my feedback, once I get some time!

@mohammedzamakhan
Copy link
Contributor

@Krustie101 can you also look into using this pattern for assignModelValue in utils.ts#L67 and getValueForKey in utils.ts#L83

@Krustie101
Copy link
Contributor Author

@mohammedzamakhan I will do, but I am tied up tomorrow. I will look at in on Thursday.

@mohammedzamakhan mohammedzamakhan merged commit c6b8892 into ngx-formly:master Nov 29, 2016
@Krustie101
Copy link
Contributor Author

Krustie101 commented Dec 1, 2016

The problem lies in the use of formControlName in the FormlyFieldRadio component, this cannot be a nested path. Like in the case of an input the formControlName should be removed and [formControl]="formControl" should be used instead.

I also notice on line 56 of the FormBuilder (master branch) the following check:

if (!form.get(field.key)) {

Should this not be

if (!form.get(rootPath)) {

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.

5 participants