Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

feat(ng-model-options) Added ng-model-options #974

Closed
wants to merge 7 commits into from
Closed

feat(ng-model-options) Added ng-model-options #974

wants to merge 7 commits into from

Conversation

jrote1
Copy link
Contributor

@jrote1 jrote1 commented Apr 28, 2014

Add support for the debounce part of ng-model-options

@jrote1
Added ng-model-options support
fcfc248 
Add support for the debounce part of ng-model-options
vicb
vicb reviewed Apr 28, 2014
View reviewed changes
lib/directive/ng_model.dart
_expression = attrs["ng-model"];
print("model: " + _expression);
Copy link
Contributor

Choose a reason for hiding this comment

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

debug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry will remove that.

@mhevery
Copy link
Contributor

mhevery commented Apr 28, 2014

Thanks for your contribution! In order for us to be able to accept it, we ask you to sign our CLA (contributor's license agreement). CLA is important for us to be able to avoid legal troubles down the road.

For individuals (a simple click-through form): http://code.google.com/legal/individual-cla-v1.0.html

@jrote1 jrote1 added cla: no and removed cla: yes labels Apr 28, 2014
@vicb
Copy link
Contributor

vicb commented Apr 28, 2014

I have started to add some comments in the PR. Some of them are repeated items, please check all changed lines where applicable.

Please also follow the guidelines in /CONTRIBUTE.md for commit messages - it should also include "closes #969".

Thanks for your work!

@jrote1
Copy link
Contributor Author

jrote1 commented Apr 28, 2014

Have signed the CLA

@jrote1
feat(ng-model-options) Added ng-model-options
c1a2cc4 
Added support for the debounce part of ng-model-options
closes #969
@jrote1
Copy link
Contributor Author

jrote1 commented Apr 28, 2014

Think I have done everything asked if there is anything else let me know and I will fix ASAP, sorry for any problems it is my first time contributing to this project. Would you provide an eta when this would be released, as I have added this as i require it in a project I am working on.

@jrote1 jrote1 changed the title Added ng-model-options support feat(ng-model-options) Added ng-model-options Apr 28, 2014
vicb
vicb reviewed Apr 28, 2014
View reviewed changes
lib/directive/ng_model.dart
@@ -43,10 +43,9 @@ class NgModel extends NgControl implements AttachAware {
Watch _watch;
bool _watchCollection;

NgModel(this._scope, NgElement element, Injector injector, NodeAttrs attrs,
NgModel(this._scope, NgElement element, Injector injector, NodeAttrs attrs,
Copy link
Contributor

Choose a reason for hiding this comment

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

extra white space

@vicb
Copy link
Contributor

vicb commented Apr 28, 2014

Thanks for fixing most of the first batch of comments, I've added some more.

Please squash your commits and force push once you're done

vicb
vicb reviewed Apr 28, 2014
View reviewed changes
lib/directive/ng_model.dart
@@ -45,7 +45,7 @@ class NgModel extends NgControl implements AttachAware {

NgModel(this._scope, NgElement element, Injector injector, NodeAttrs attrs,
Animate animate)
: super(element, injector, animate)
: super(element, injector, animate)
Copy link
Contributor

Choose a reason for hiding this comment

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

you should configure your editor to remove trailing ws

vicb
vicb reviewed Apr 28, 2014
View reviewed changes
lib/directive/ng_model.dart
final Scope scope;
NgBindTypeForDateLike ngBindType;

InputDateLike(dom.Element this.inputElement, this.ngModel, this.scope,
this.ngBindType) {
InputDateLike(dom.Element this.inputElement, this.ngModel, this.scope, this.ngBindType, this.ngModelOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

too long

@mhevery
Copy link
Contributor

mhevery commented Apr 28, 2014

Achievement unlocked: CLA signature found!

vicb
vicb reviewed Apr 28, 2014
View reviewed changes
lib/directive/ng_model_options.dart
func();
}
else{
setTimer(new async.Timer(new Duration(milliseconds: delay), func));
Copy link
Contributor

Choose a reason for hiding this comment

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

what about dropping the 3rd args & returning the timer

@jrote1 jrote1 added cla: yes and removed cla: no labels Apr 28, 2014
vicb
vicb reviewed Apr 28, 2014
View reviewed changes
lib/directive/ng_model_options.dart
void _runFuncDebounced(int delay, func(), setTimer(async.Timer timer), async.Timer timer){
if (timer != null && timer.isActive) timer.cancel();

if(delay == 0){
Copy link
Contributor

Choose a reason for hiding this comment

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

if () {
} else {
}

notice ws & formatting

@jrote1
Copy link
Contributor Author

jrote1 commented Apr 28, 2014

Think thats all of them

@jrote1
Copy link
Contributor Author

jrote1 commented Apr 28, 2014

Will remember all of this for when I do future additions to reduce all of these issues.

vicb
vicb reviewed Apr 28, 2014
View reviewed changes
lib/directive/ng_model.dart
@@ -196,7 +195,7 @@ class NgModel extends NgControl implements AttachAware {

get viewValue => _viewValue;
void set viewValue(value) {
_viewValue = value;
_viewValue = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

revert

@vicb
Copy link
Contributor

vicb commented Apr 28, 2014

Do you think you can come with a unit test ?

@jrote1
Copy link
Contributor Author

jrote1 commented Apr 28, 2014

Once this is merged how long untill it will be released?

Will sort all of the test stuff out tommorow.

@vicb
Copy link
Contributor

vicb commented Apr 29, 2014

Once this is merged how long untill it will be released?

Sorry I don't have the release planning in mind. Before it is released, you can point your dependency to a branch (master) or a SHA1.

zoechi
zoechi reviewed Apr 30, 2014
View reviewed changes
lib/directive/ng_model_options.dart
@@ -0,0 +1,62 @@
part of angular.directive;

@Decorator(selector: 'input[ng-model-options]')
Copy link
Contributor

Choose a reason for hiding this comment

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

Some doc about the purpose of this class would be great!

mhevery pushed a commit that referenced this pull request May 8, 2014
@jrote1 @mhevery
feat(ng-model): Added ng-model-options
3faf59b 
Added support for the debounce part of ng-model-options

Closes #969
Closes #974
@matsko matsko closed this in f7115aa May 8, 2014
jbdeboer pushed a commit to jbdeboer/angular.dart that referenced this pull request May 13, 2014
@jrote1 @jbdeboer
feat(ng-model): Added ng-model-options
91c9465 
Added support for the debounce part of ng-model-options

Closes dart-archive#969
Closes dart-archive#974
dsalsbury pushed a commit to dsalsbury/angular.dart that referenced this pull request Jul 16, 2014
@jrote1
feat(ng-model): Added ng-model-options
1acf9bd 
Added support for the debounce part of ng-model-options

Closes dart-archive#969
Closes dart-archive#974
dsalsbury pushed a commit to dsalsbury/angular.dart that referenced this pull request Jul 16, 2014
@jrote1
feat(ng-model): Added ng-model-options
2fddbad 
Added support for the debounce part of ng-model-options

Closes dart-archive#969
Closes dart-archive#974
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jrote1 @mhevery @vicb @zoechi