[New Feature] Add new possible value for ngrxUpdateOn #119

Mr-Eluzive · 2018-10-17T17:51:22Z

This PR address my proposal #118.

<!-- Example usage -->
<input [ngrxFormState]="formState$ | async" ngrxUpdateOn="never" />

codecov-io · 2018-10-17T17:55:45Z

Codecov Report

Merging #119 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #119   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         116    116           
  Lines        2180   2184    +4     
  Branches      408    408           
=====================================
+ Hits         2180   2184    +4

Impacted Files	Coverage Δ
src/control/directive.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 808677a...9d66add. Read the comment docs.

MrWolfZ · 2018-10-17T22:14:33Z

Thank you for this PR.

I can see how this feature would be useful. However, I am not so sure about still dispatching the MarkAsDirtyAction when never is used. With change and blur the SetValueAction and MarkAsDirtyAction are always dispatched together (if the state is still pristine) because if there is no SetValueAction, then the form state is not really dirty, even if the HTML element is.

There is also the problem of when to dispatch the MarkAsDirtyAction with never. In your implementation you do it in the blur handler, but it could just as well be in the input handler.

Therefore, to prevent confusion I would like you to change the code to simply not dispatch either action if never is chosen. Please also update the documentation accordingly, i.e.

"It is possible to control when view value changes are pushed to the state with the ngrxUpdateOn attribute. The supported values are change (pushed immediately when the view value changes; default), blur (pushed when the form element loses focus), and never (the value is never pushed to the state; this is an advanced feature that is useful if you want full control over when and how the state is updated but it also requires greater understanding of how ngrx-forms performs state updates). Note that by changing this value to something different than change (and thereby changing the time at which value changes are pushed to the state) you are also changing the time at which validation and other state updates that depend on the value happen. If you change this value to never you will need to perform all state updates yourself (e.g. setting the value, marking as dirty etc.)."

Mr-Eluzive · 2018-10-18T05:30:28Z

Yeah you are right. I've already changed it and squashed into one commit (quickly from another computer via HTTPS so something with my username and avatar is wrong). Could you check if it's all right now, please?

MrWolfZ

Please have a look at my comments. They are just small things, but they are important.

MrWolfZ · 2018-10-18T07:27:43Z

src/control/directive.spec.ts

+      directive.ngrxUpdateOn = NGRX_UPDATE_ON_TYPE.NEVER;
+    });
+
+    it(`should not dispatch any even if the view value changed`, done => {


The word action is missing here, i.e. should not dispatch any action even if the view value changed

MrWolfZ · 2018-10-18T07:29:21Z

docs/user-guide/form-controls.md

@@ -64,7 +64,7 @@ A constant `NGRX_STATUS_CLASS_NAMES` is exported to allow accessing these class

 #### Choosing when to sync the view to the state

-It is possible to control when view values changes are pushed to the state with the `ngrxUpdateOn` attribute. The supported values are `change` (pushed immediately when the view value changes; default) and `blur` (pushed when the form element loses focus). Note that by changing this value to something different than `change` (and thereby changing the time at which value changes are pushed to the state) you are also changing the time at which validation and other state updates that depend on the value happen.
+It is possible to control when view values changes are pushed to the state with the `ngrxUpdateOn` attribute. The supported values are `change` (pushed immediately when the view value changes; default), `blur` (pushed when the form element loses focus), and `never` (the value is never pushed to the state; this is an advanced feature that is useful if you want full control over when and how the state is updated but it also requires greater understanding of how ngrx-forms performs state updates). Note that by changing this value to something different than `change` (and thereby changing the time at which value changes are pushed to the state) you are also changing the time at which validation and other state updates that depend on the value happen. If you change this value to never you will need to perform all state updates yourself (e.g. setting the value, marking as dirty etc.).


In the documentation whenever I mention ngrx-forms, I make it bold. So to be consistent please change this to use **ngrx-forms**

src/control/directive.ts

MrWolfZ · 2018-10-18T07:38:31Z

src/control/directive.spec.ts

+  describe('ngrxUpdateOn "never"', () => {
+    beforeEach(() => {
+      directive.ngOnInit();
+      directive.ngrxFormControlState = { ...INITIAL_STATE, isTouched: true, isUntouched: false };


This is not necessary here since it leads to your additional check for the update on type not being executed. Just remove this line. The test should still work without it.

Mr-Eluzive · 2018-10-18T18:15:47Z

Ok it looks like I've correct all your suggestions. Please check this again and let me know.

MrWolfZ · 2018-10-21T08:12:55Z

Sorry, for the delay. I am on vacation right now so there are bound to be some delays. However, I'll release a new version with this change right now.

Thank you again for this PR.

MrWolfZ requested changes Oct 18, 2018

View reviewed changes

Add new possible value for ngrxUpdateOn

9d66add

MrWolfZ approved these changes Oct 21, 2018

View reviewed changes

MrWolfZ merged commit ea0b284 into MrWolfZ:master Oct 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New Feature] Add new possible value for ngrxUpdateOn #119

[New Feature] Add new possible value for ngrxUpdateOn #119

Mr-Eluzive commented Oct 17, 2018

codecov-io commented Oct 17, 2018 •

edited

Loading

MrWolfZ commented Oct 17, 2018

Mr-Eluzive commented Oct 18, 2018 •

edited

Loading

MrWolfZ left a comment

MrWolfZ Oct 18, 2018

MrWolfZ Oct 18, 2018

MrWolfZ Oct 18, 2018

Mr-Eluzive commented Oct 18, 2018

MrWolfZ commented Oct 21, 2018

[New Feature] Add new possible value for ngrxUpdateOn #119

[New Feature] Add new possible value for ngrxUpdateOn #119

Conversation

Mr-Eluzive commented Oct 17, 2018

codecov-io commented Oct 17, 2018 • edited Loading

Codecov Report

MrWolfZ commented Oct 17, 2018

Mr-Eluzive commented Oct 18, 2018 • edited Loading

MrWolfZ left a comment

Choose a reason for hiding this comment

MrWolfZ Oct 18, 2018

Choose a reason for hiding this comment

MrWolfZ Oct 18, 2018

Choose a reason for hiding this comment

MrWolfZ Oct 18, 2018

Choose a reason for hiding this comment

Mr-Eluzive commented Oct 18, 2018

MrWolfZ commented Oct 21, 2018

codecov-io commented Oct 17, 2018 •

edited

Loading

Mr-Eluzive commented Oct 18, 2018 •

edited

Loading