fix(popover-edit): incorrectly caching values for more than one control #16057
Conversation
crisbeto
added
pr: merge safe
target: patch
googlebot
added
the
cla: yes
| @@ -41,7 +41,8 @@ export class EditRef<FormValue> implements OnDestroy { | |||
|
|
|||
| constructor( | |||
| @Self() private readonly _form: ControlContainer, | |||
| private readonly _editEventDispatcher: EditEventDispatcher) { | |||
| private readonly _editEventDispatcher: EditEventDispatcher, | |||
| private _ngZone: NgZone) { | |||
There was a problem hiding this comment.
I don't think I'm on board with marking everything as readonly. This is a private property which is very unlikely to be changed by something internal. We haven't done it anywhere else in the library and we haven't run into any issues with it so far.
There was a problem hiding this comment.
It's correct, though; we should be marking readonly way more than we do, we just never got in the habit of it
There was a problem hiding this comment.
Changed, although I feel like it's extremely unlikely that it'll actually catch anything. Furthermore since this is already private, if somebody wanted to override for some reason, there's nothing stopping them from just removing the readonly.
crisbeto
force-pushed
the
popover-edit-revert
branch
2 times, most recently
from
83843c7 to
840181c
Compare
crisbeto
added
the
P2
crisbeto
force-pushed
the
popover-edit-revert
branch
from
840181c to
adea7dc
Compare
Currently the edit popover uses the first `valueChange` event to know when to cache the value of its form so that it can revert to it if the user presess the `revert` button. This breaks down if the form has more than one control, because it'll emit a `valueChange` for each control that gets added. These changes fix the issue by using `NgZone` to wait for the form to be initialized.
crisbeto
force-pushed
the
popover-edit-revert
branch
from
adea7dc to
369d1b6
Compare
|
LGTM |
…ol (#16057) Currently the edit popover uses the first `valueChange` event to know when to cache the value of its form so that it can revert to it if the user presess the `revert` button. This breaks down if the form has more than one control, because it'll emit a `valueChange` for each control that gets added. These changes fix the issue by using `NgZone` to wait for the form to be initialized.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently the edit popover uses the first
valueChangeevent to know when to cache the value of its form so that it can revert to it if the user presess therevertbutton. This breaks down if the form has more than one control, because it'll emit avalueChangefor each control that gets added. These changes fix the issue by usingNgZoneto wait for the form to be initialized.