search

formly-js / angular-formly

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

feat(formly-form): add 'watcher' to fieldGroup #610

Closed benoror closed 8 years ago

benoror commented 8 years ago

originally requested in: http://stackoverflow.com/questions/34850697/clearing-fieldgroups-fields-when-hidden/34857087?noredirect=1#comment57499232_34857087

codecov-io commented 8 years ago

Current coverage is 95.90%

Merging #610 into master will increase coverage by +0.02% as of 80a6bb1

@@            master   #610   diff @@
=====================================
  Files           16     16       
  Stmts         1117   1122     +5
  Branches         0      0       
  Methods          0      0       
=====================================
+ Hit           1071   1076     +5
  Partial          0      0       
  Missed          46     46

Review entire Coverage Diff as of 80a6bb1

Uncovered Suggestions

  1. +0.35% 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

@benoror looks like you updated the PR. Looks good to me. Anyone from @formly-js/angular-formly-collaborators or @formly-js/angular-formly-collaborators-read wanna review this and push the merge button?

benoror commented 8 years ago

I did, I forgot that github doesn't notify PR updates

kamilkisiela commented 8 years ago

@benoror Are you going to resolve conflicts or should I look it up?

kentcdodds commented 8 years ago

Sorry, I just merged another PR :cry:

benoror commented 8 years ago

Np, give me a moment :smile:

benoror commented 8 years ago

@kamilkisiela Apparently the last PR changed apiCheck for watchers, among other stuff, thus now I get some unit tests failing.

Sorry, this merge is going to take a bit more time, feel free to try it yourself meanwhile

kentcdodds commented 8 years ago

Sorry about that @benoror.

kamilkisiela commented 8 years ago

@benoror Saw that too. Ok :)

kamilkisiela commented 8 years ago

Ok, done.

I also fixed one error caused by one of dependencies in karma cfaa247

kentcdodds commented 8 years ago

:clap:

benoror commented 8 years ago

@kamilkisiela Great! :smile:

You reverted some of the changes, for example: I introduced watcherChecker, then you reverted it back :cry:

Since you also introduced several more commits, do you mind cleaning the git history a little bit?

Thanks!

kamilkisiela commented 8 years ago

@benoror I just merged your branch and then fix conflicts.

watcherChecker is great but it will work only with fieldGroup's apiChecking.

fieldOptionsApiShape has one more option runFieldExpressions.

I think that ApiCheck has no "extending" function so this watcherChecker would be used only in one place.

Correct me if I'm wrong.

About mess in git history :) I'm going to work on it tomorrow.

benoror commented 8 years ago

maybe the new runFieldExpressions property should be in both places?

kamilkisiela commented 8 years ago

@kentcdodds What do you think?

kentcdodds commented 8 years ago

Ah, yes, probably should.

kamilkisiela commented 8 years ago

@benoror Done :)

benoror commented 8 years ago

@kamilkisiela thanks a lot! :tada: