Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

docs(ngModel.NgModelController): describe your change... #15272

Closed
wants to merge 2 commits into from
Closed

docs(ngModel.NgModelController): describe your change... #15272

wants to merge 2 commits into from

Conversation

juboba
Copy link
Contributor

@juboba juboba commented Oct 15, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bug fix: The model should be cleared out only if we don't allow rollback.

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

Currently, the model is always being cleared out to ''

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

The new behaviour matches the docs.

Does this PR introduce a breaking change?

No

Please check if the PR fulfills these requirements

Other information:

If there's no else, then the same will happen in both inputs (the model value is overwritten).

If there's no else, then the same will happen in both inputs (the model value is overwritten).
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@juboba
Copy link
Contributor Author

juboba commented Oct 15, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@gkalpak
Copy link
Member

gkalpak commented Oct 15, 2016

Although the example is broken, this fix is not correct. As described above the example, what it tries to showcase is setting the model (i.e. scope.value1) to some value programmatically when there is a pending update.

So, in both cases, it must set the model to an empty string and the difference is that when your don't cancel the pending update, it will overwrite the empty string you set once the field is blurred.

In order to see that, the model value has to already be the empty string, which is not the case in this example. That's why it seems/is broken.

So, the proper fix is initializing both models to an empty string inside the controller:

$scope.model = {value1: '', value2: ''};

@juboba, would you like to update your PR with the above change?

@juboba
Copy link
Contributor Author

juboba commented Oct 15, 2016

@gkalpak yes, I'd like to update my PR, but I'm checking in this in plnkr:

https://plnkr.co/edit/ZtqpkoOhHWdJQwOTF9mY?p=preview

You can see the value is being reset to the previous one, but the model is then set to '', so the viewValue is also changed to ''

@gkalpak
Copy link
Member

gkalpak commented Oct 15, 2016

If you remove the alert (which takes the focus and causes the field to blur prematurely) it works as expected for me.

@Narretz
Copy link
Contributor

Narretz commented Oct 31, 2016

@juboba Are you planning on completing the changes for this PR?

@juboba
Copy link
Contributor Author

juboba commented Nov 1, 2016

Hi Martin,

I'm confused with Georgios Kalpakas' reply. Do you agree that the expected
result is obtained when making his changes?

"Injustice anywhere is a threat to justice everywhere." -- Martin Luther
King, Jr

On Mon, Oct 31, 2016 at 6:00 PM, Martin Staffa notifications@github.com
wrote:

@juboba https://github.com/juboba Are you planning on completing the
changes for this PR?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15272 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABInawZDpq55C9E-GwFUHlH-UhrH7QVeks5q5h6sgaJpZM4KXoyP
.

@Narretz
Copy link
Contributor

Narretz commented Nov 5, 2016

Yes, I think for the purpose of this example, @gkalpak's explanation is correct. It's a very specific scenario that shows that you need $rollBackViewValue in some cases when you have non-immediate scope updates.

Update commit to match @gkalpak 's observations.
@juboba
Copy link
Contributor Author

juboba commented Nov 11, 2016

I updated my commit according to @gkalpak's explanation.

@gkalpak gkalpak closed this in 5419201 Nov 14, 2016
gkalpak pushed a commit that referenced this pull request Nov 14, 2016
For the example to work correctly, the initial model values have to be empty strings.

Closes #15272
petebacondarwin pushed a commit to petebacondarwin/angular.js that referenced this pull request Nov 21, 2016
For the example to work correctly, the initial model values have to be empty strings.

Closes angular#15272
petebacondarwin pushed a commit to petebacondarwin/angular.js that referenced this pull request Nov 21, 2016
For the example to work correctly, the initial model values have to be empty strings.

Closes angular#15272
petebacondarwin pushed a commit to petebacondarwin/angular.js that referenced this pull request Nov 21, 2016
For the example to work correctly, the initial model values have to be empty strings.

Closes angular#15272
petebacondarwin pushed a commit to petebacondarwin/angular.js that referenced this pull request Nov 21, 2016
For the example to work correctly, the initial model values have to be empty strings.

Closes angular#15272
petebacondarwin pushed a commit to petebacondarwin/angular.js that referenced this pull request Nov 21, 2016
For the example to work correctly, the initial model values have to be empty strings.

Closes angular#15272
petebacondarwin pushed a commit that referenced this pull request Nov 23, 2016
For the example to work correctly, the initial model values have to be empty strings.

Closes #15272
petebacondarwin pushed a commit that referenced this pull request Nov 24, 2016
For the example to work correctly, the initial model values have to be empty strings.

Closes #15272
ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017
For the example to work correctly, the initial model values have to be empty strings.

Closes angular#15272
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants