formly-js / angular-formly

JavaScript powered forms for AngularJS
http://docs.angular-formly.com
MIT License
2.23k stars 405 forks source link

fix(formly-form): added originalModel and formOptions to getFormlyFie… #626

Closed nikhilshagri closed 8 years ago

nikhilshagri commented 8 years ago

…ldLikeLocals function

added originalModel and formOptions to getFormlyFieldLikeLocals function

issue : https://github.com/formly-js/angular-formly/issues/616

codecov-io commented 8 years ago

Current coverage is 96.00%

Merging #626 into master will not affect coverage as of bdea08d

@@            master   #626   diff @@
=====================================
  Files           16     16       
  Stmts         1150   1150       
  Branches         0      0       
  Methods          0      0       
=====================================
  Hit           1104   1104       
  Partial          0      0       
  Missed          46     46       

Review entire Coverage Diff as of bdea08d


Uncovered Suggestions

  1. +0.34% via ...alidationMessages.js#25...28
  2. +0.26% via ...s/formlyUsability.js#18...20
  3. +0.26% via src/test.utils.js#38...40
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

kentcdodds commented 8 years ago

Awesome! Mind adding a test or two that verifies these changes work (and to prevent them from breaking in the future?)

nikhilshagri commented 8 years ago

Sure, why not? Some tips on how to go about it would be useful :)

kentcdodds commented 8 years ago

Sure! :+1:

I'd probably put it in the section of tests that test hideExpression because that's one of the main places that leverages this feature. Find that here.

I would take the existing beforeEach and two it blocks and nest them in another describe with a title of something like: behavior when the model changes

Then I would create another describe block for your tests with the title of something like as a string. And then add an it with the message: should have access to almost all the same properties as an expressionProperties string expression.

Then in your test, you'll want to initialize scope.fields, scope.model, and scope.options (scope.options should have a formState object). And for scope.fields you can use the getNewField helper function (see other places for how that's used. It's pretty much just a helper so you can omit the template/type/etc.).

It may be a bit tricky because you can't really do assertions in string expressions, but the returned value of the string expression is assigned to the field's hide property. So you might try something like this:

scope.model = {}
scope.options = {formState: {}}
scope.fields = [getNewField({
  hideExpression: `
    options === options.data.field &&
    index === 0 &&
    formState === options.data.formOptions.formState &&
    originalModel === options.data.originalModel &&
    formOptions === options.data.formOptions
  `, // can't reliably test `formId` unfortunately
  data: {
    originalModel: scope.model,
    formOptions: scope.options,
  }
})]
scope.fields[0].data.field = scope.fields[0]

Then invoke compileAndDigest and assert that the scopes.fields[0].hide is true.

Let me know what you think or feel free to change whatever you like. Looking forward to seeing this. Thanks!

nikhilshagri commented 8 years ago

Awesome! I will get cracking on the tests right away :+1:

nikhilshagri commented 8 years ago

What is the field's data property actually used for? The documentation is a bit vague about it...

kentcdodds commented 8 years ago

It's for anything you want. Because angular-formly validates all of the properties you provide, if you have any other things you need to provide, they either go in templateOptions (if it's specific for the template or type that you're using) or in data if it's just general data that you need to get your job done (like in this case).

nikhilshagri commented 8 years ago

Thanks. Just a couple more questions:

  1. We can also pass a function to hideExpression to evaluate it, but there are no tests for it. Should I add another test for it?
  2. Should I commit these tests to a separate commit or should I squash it with the previous commit into a single one?
kentcdodds commented 8 years ago

I'm fine with either way. Feel free to add more tests in this pr, feel free to squash if you like.

kentcdodds commented 8 years ago

If you could fix that comment and the name of that data property (to parentScope maybe), then this is great and I'll merge it. Thanks!

nikhilshagri commented 8 years ago

I have updated the PR with the changes you requested.

kentcdodds commented 8 years ago

Perfect. Thank you!