Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

feat(ngModelOptions): support submit trigger #7094

Closed
wants to merge 1 commit into from
Closed

feat(ngModelOptions): support submit trigger #7094

wants to merge 1 commit into from

Conversation

shahata
Copy link
Contributor

@shahata shahata commented Apr 11, 2014

Fixes #7017

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#7094)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@shahata
Copy link
Contributor Author

shahata commented Apr 12, 2014

I thought it would be better to broadcast the submit event on the scope instead of letting each input listen on the form's submit itself. This is much more elegant in my opinion since you don't need to pass the form's element to the input directives. (which is very patchy, not to mention nested forms which will require particular ugly patching)

@petebacondarwin @lrlopez what do you think about this? Should I also document the new $formSubmit event?

@lrlopez
Copy link
Contributor

lrlopez commented Apr 12, 2014

SGTM. I also find convenient to have both the event and the new feature documented. As you stated before, an input control doesn't have a submit DOM event so we should explicitly state that is possible to use it. Good work!

* events using an space delimited list. There is a special event called `default` that
* events using an space delimited list. You can also ask to update on `submit`, but make sure
* you handle the submit using `ngSubmit` and not `ngClick` on the submit button in order to
* have the updated model value on your scope. There is a special event called `default` that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should say:

  • updateOn: string a space separated list of events; any of which should cause the model to be updated.
    There are two "special" events:
    • default: the standard built-in update event for this input (e.g. change or input).
    • submit: triggered when an enclosing form is submitted via the submit event. Note that ngClick events will occur before the model is updated. Use ngSubmit to have access to the updated model.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not catch a click on an input of type submit?

Sorry I misread the doc

@petebacondarwin
Copy link
Member

This PR is great @shahata!

I agree with @lrlopez about documenting it in both places. Also I suggested an alternative function name and wording for one of the docs.

I would like to get another core team member to take a look before we merge - @auser?

@petebacondarwin
Copy link
Member

This is failing e2e tests locally for me:

Failures:

  1) module:ng.directive:ngModelOptions should allow custom events
   Message:
     Expected 'say' to equal 'say hello'.
   Stacktrace:
     Error: Expected 'say' to equal 'say hello'.
    at /Users/pete/dev/angular/angular.js/node_modules/protractor/jasminewd/index.js:95:30

  2) module:ng.directive:ngModelOptions should allow custom events
   Message:
     Expected 'say' to equal 'say hello'.
   Stacktrace:
     Error: Expected 'say' to equal 'say hello'.
    at /Users/pete/dev/angular/angular.js/node_modules/protractor/jasminewd/index.js:95:30

This looks like the failures we had before due to timing issues... @shahata - did you branch from an old version?

@petebacondarwin
Copy link
Member

Or maybe it is me! I'll push to CI and see.

@petebacondarwin
Copy link
Member

OK, so Travis and Jenkins are happy with this. Must be something wrong on my end.

@petebacondarwin
Copy link
Member

OK, builds now for me locally too. @caitp raised a concern on Gitter, which I think would be dealt with by fleshing out the reasoning behind this change in more detail in the docs, in ng-model-options and for form, input and NgModelController.

@shahata
Copy link
Contributor Author

shahata commented Apr 13, 2014

@petebacondarwin I've updated the commit so that update on submit will be the default. (currently there is no way to disable it, but it can be done very easily if we find a reason to do it)

Also:

  1. Documented the $flushPendingInput event so ppl can trigger it manually.
  2. Fixed the implementation to also flush debounced events, which I forgot about the first time around.
  3. Allowed updateOn to be an empty string in case you want the model to only update on submit.
  4. Since ppl are likely to trigger $flushPendingInput from within scope.$apply, I had to test scope.$$phase in all listeners. I don't like this, but other workarounds might be even less pretty... What do you think?
  5. Pay attention to how addUpdateOnListeners invokes the listener. This is somewhat similar to the previous item in this list. Maybe I should just invoke $$realSetViewValue directly from there somehow. Still thinking about it.

Do you think we should add documentation for this elsewhere? In my opinion the right place to document this is only ngModelOptions...

@@ -381,19 +381,21 @@ var formDirectiveFactory = function(isNgForm) {
// IE 9 is not affected because it doesn't fire a submit event and try to do a full
// page reload if the form was destroyed by submission of the form via a click handler
// on a button in the form. Looks like an IE9 specific bug.
var preventDefaultListener = function(event) {
var handleFormSubmission = function(event) {
scope.$broadcast('$flushPendingInput', event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we run this using scope.$evalAsync? This will guarantee that the handler is called from within a $digest cycle.

We should then expect anyone else who is manually triggering this event to ensure that they are inside a $digest cycle themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why scope.$evalAsync and not scope.$apply? I'm afraid that if we use scope.$evalAsync the event will be handled too late (after ngSubmit is invoked)

@shahata
Copy link
Contributor Author

shahata commented Apr 13, 2014

PR updated according to @petebacondarwin comments

@lefos987 lefos987 added this to the 1.3.0 milestone Apr 14, 2014
@shahata
Copy link
Contributor Author

shahata commented Apr 15, 2014

I don't like some things about this PR, mainly all the tweaking with scope.$apply and the fact that each input directive must repeat a lot of logic in order to work with ngModelOptions. I think #7116 is a better solution. What do you think?

@petebacondarwin
Copy link
Member

I will take a look next week.

On 15 April 2014 09:43, Shahar Talmi notifications@github.com wrote:

I don't like some things about this PR, mainly all the tweaking with
scope.$apply and the fact that each input directive must repeat a lot of
logic in order to work with ngModelOptions. I think #7116https://github.com/angular/angular.js/pull/7116is a better solution. What do you think?

Reply to this email directly or view it on GitHubhttps://github.com//pull/7094#issuecomment-40458235
.

@shahata shahata added cla: yes and removed cla: no labels Apr 18, 2014
@shahata
Copy link
Contributor Author

shahata commented Apr 28, 2014

BTW, I have a different PR - #7116 which solves the same issue in a different way and I really prefer instead of this one.

@petebacondarwin petebacondarwin modified the milestones: 1.3.0-beta.8, 1.3.0 Apr 28, 2014
@petebacondarwin petebacondarwin self-assigned this Apr 28, 2014
@shahata
Copy link
Contributor Author

shahata commented May 3, 2014

Closing in favor of #7116

@shahata shahata closed this May 3, 2014
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.

Support 'submit' updateOn trigger for forms when using ng-model-options
5 participants