New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ngOptions): add support for disabling an option #11017

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@sjbarker
Contributor

sjbarker commented Feb 9, 2015

This patch adds support for disabling options based on model values. The
"disable when" syntax allows for listening to changes on those model values,
in order to dynamically enable and disable the options.

The changes prevent disabled options from being written to the selectCtrl
from the model. If a disabled selection is present on the model, normal
unknown or empty functionality kicks in.

closes #638

@googlebot googlebot added the cla: yes label Feb 9, 2015

@sjbarker

This comment has been minimized.

Show comment
Hide comment
@sjbarker

sjbarker Feb 9, 2015

Contributor

Explanation

The changes in this pull request reflect a long-time reoccurring and widely popular request for the ability to disable options dynamically in ngOptions. This solution accomplishes this feature by adding the disable when syntax to the ng-options directive. All current functionality is retained in select controls.

The solution for model changes reflecting disabled options is to keep the current functionality of "unknown" or "empty" options. Tests have been written to reflect this feature. Consider the following:

scope.selected = '';
scope.options = [ 'a', 'b', 'c', 'd'];
scope.disableOption = function(option) {
  return option === 'b';
}
<select ng-model="selected"
             ng-options="o disable when disableOption(o) for o in options"></select>

-OR-

scope.selected = '';
scope.options = [ 
  {
    name: 'a',
    disabled: false
  },
  {
    name: 'b',
    disabled: false
  },
  {
    name: 'c',
    disabled: false
  },
  {
    name: 'd',
    disabled: false
  }
];
<select ng-model="selected"
             ng-options="o.name disable when o.disabled for o in options"></select>

In either case, if selected becomes a disabled option, then select will render the empty or unknown option. This concurrent with any unknown or invalid option.

Current Issues

Issue #638 has been open for over three years now and is currently on backlog.

Current PR's

There is one current PR (#9854) by @cades that has been rendered obsolete by @petebacondarwin's select.js refactor (408f89d).

Contributor

sjbarker commented Feb 9, 2015

Explanation

The changes in this pull request reflect a long-time reoccurring and widely popular request for the ability to disable options dynamically in ngOptions. This solution accomplishes this feature by adding the disable when syntax to the ng-options directive. All current functionality is retained in select controls.

The solution for model changes reflecting disabled options is to keep the current functionality of "unknown" or "empty" options. Tests have been written to reflect this feature. Consider the following:

scope.selected = '';
scope.options = [ 'a', 'b', 'c', 'd'];
scope.disableOption = function(option) {
  return option === 'b';
}
<select ng-model="selected"
             ng-options="o disable when disableOption(o) for o in options"></select>

-OR-

scope.selected = '';
scope.options = [ 
  {
    name: 'a',
    disabled: false
  },
  {
    name: 'b',
    disabled: false
  },
  {
    name: 'c',
    disabled: false
  },
  {
    name: 'd',
    disabled: false
  }
];
<select ng-model="selected"
             ng-options="o.name disable when o.disabled for o in options"></select>

In either case, if selected becomes a disabled option, then select will render the empty or unknown option. This concurrent with any unknown or invalid option.

Current Issues

Issue #638 has been open for over three years now and is currently on backlog.

Current PR's

There is one current PR (#9854) by @cades that has been rendered obsolete by @petebacondarwin's select.js refactor (408f89d).

@sjbarker

This comment has been minimized.

Show comment
Hide comment
@sjbarker

sjbarker Feb 9, 2015

Contributor

@petebacondarwin - Since you recently did the select directive refactor, will you take a quick look at this and pass it around? We have been sitting on this for a really long time and this would provide the functionality without breaking anything; and with room to grow in the future. Thanks!

Contributor

sjbarker commented Feb 9, 2015

@petebacondarwin - Since you recently did the select directive refactor, will you take a quick look at this and pass it around? We have been sitting on this for a really long time and this would provide the functionality without breaking anything; and with room to grow in the future. Thanks!

@cades

This comment has been minimized.

Show comment
Hide comment
@cades

cades Feb 10, 2015

Great job, thanks to your effort and time!
Hope to merge this PR as soon as possible.

cades commented Feb 10, 2015

Great job, thanks to your effort and time!
Hope to merge this PR as soon as possible.

@sjbarker

This comment has been minimized.

Show comment
Hide comment
@sjbarker

sjbarker Feb 10, 2015

Contributor

Thanks @cades. You were there first until they refactored the select directive. And thank you for your time and effort as well.

Contributor

sjbarker commented Feb 10, 2015

Thanks @cades. You were there first until they refactored the select directive. And thank you for your time and effort as well.

@sjbarker

This comment has been minimized.

Show comment
Hide comment
@sjbarker

sjbarker Feb 10, 2015

Contributor

@pkozlowski-opensource - I figured I would tag you in this since you were attending to the #9854 PR. When you have a moment, will you take a look as well? Thanks.

Contributor

sjbarker commented Feb 10, 2015

@pkozlowski-opensource - I figured I would tag you in this since you were attending to the #9854 PR. When you have a moment, will you take a look as well? Thanks.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Feb 12, 2015

Member

OK, I like this idea, although I think that the syntax sound better with disable when rather than disable by.

Also I am concerned about add to the getWatchables array, which is already a bit of a pain point.

I realised that we can actually optimize this by only including things that are being specified. So for instance, if no disable when clause was provided we would not need to watch this expression. The same for the displayFn which computes the label.

Member

petebacondarwin commented Feb 12, 2015

OK, I like this idea, although I think that the syntax sound better with disable when rather than disable by.

Also I am concerned about add to the getWatchables array, which is already a bit of a pain point.

I realised that we can actually optimize this by only including things that are being specified. So for instance, if no disable when clause was provided we would not need to watch this expression. The same for the displayFn which computes the label.

@sjbarker

This comment has been minimized.

Show comment
Hide comment
@sjbarker

sjbarker Feb 13, 2015

Contributor

No problem, @petebacondarwin. Let me get those changes in real quick. I will open an issue for optimizing the getWatchables array and cover it in this commit as well.

Contributor

sjbarker commented Feb 13, 2015

No problem, @petebacondarwin. Let me get those changes in real quick. I will open an issue for optimizing the getWatchables array and cover it in this commit as well.

//000011111111110000000000022222222220000000000000000000003333333333000000000000004444444444444440000000005555555555555550000000666666666666666000000000000000777777777700000000000000000008888888888
var NG_OPTIONS_REGEXP = /^\s*([\s\S]+?)(?:\s+as\s+([\s\S]+?))?(?:\s+group\s+by\s+([\s\S]+?))?\s+for\s+(?:([\$\w][\$\w]*)|(?:\(\s*([\$\w][\$\w]*)\s*,\s*([\$\w][\$\w]*)\s*\)))\s+in\s+([\s\S]+?)(?:\s+track\s+by\s+([\s\S]+?))?$/;
// //00001111111111000000000002222222222000000000000000000000333333333300000000000000000000000004444444444400000000000005555555555555550000000006666666666666660000000777777777777777000000000000000888888888800000000000000000009999999999
var NG_OPTIONS_REGEXP = /^\s*([\s\S]+?)(?:\s+as\s+([\s\S]+?))?(?:\s+group\s+by\s+([\s\S]+?))?(?:\s+disable\s+when\s+([\s\S]+?))?\s+for\s+(?:([\$\w][\$\w]*)|(?:\(\s*([\$\w][\$\w]*)\s*,\s*([\$\w][\$\w]*)\s*\)))\s+in\s+([\s\S]+?)(?:\s+track\s+by\s+([\s\S]+?))?$/;

This comment has been minimized.

@sjbarker

sjbarker Feb 13, 2015

Contributor

@petebacondarwin Here is the syntax change.

@sjbarker

sjbarker Feb 13, 2015

Contributor

@petebacondarwin Here is the syntax change.

if (match[4]) {
var disableWhen = disableWhenFn(scope, locals);
watchedArray.push(disableWhen);
}

This comment has been minimized.

@sjbarker

sjbarker Feb 13, 2015

Contributor

@petebacondarwin - I rebased your perf and mimicked it with the disableWhenFn

@sjbarker

sjbarker Feb 13, 2015

Contributor

@petebacondarwin - I rebased your perf and mimicked it with the disableWhenFn

feat(ngOptions): add support for disabling an option
This patch adds support for disabling options based on model values. The
"disable when" syntax allows for listening to changes on those model values,
in order to dynamically enable and disable the options.

The changes prevent disabled options from being written to the selectCtrl
from the model. If a disabled selection is present on the model, normal
unknown or empty functionality kicks in.

closes #638
@sjbarker

This comment has been minimized.

Show comment
Hide comment
@sjbarker

sjbarker Feb 18, 2015

Contributor

@petebacondarwin I returned the ngModel requirement to optional.

Contributor

sjbarker commented Feb 18, 2015

@petebacondarwin I returned the ngModel requirement to optional.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Feb 18, 2015

Member

I added a few extra tests and merged into master - thanks @sjbarker and @cades !

Member

petebacondarwin commented Feb 18, 2015

I added a few extra tests and merged into master - thanks @sjbarker and @cades !

@sjbarker

This comment has been minimized.

Show comment
Hide comment
@sjbarker

sjbarker Feb 18, 2015

Contributor

Thanks @petebacondarwin! I didn't see that ngModel optional requirement make it in though. Is that ok?

Contributor

sjbarker commented Feb 18, 2015

Thanks @petebacondarwin! I didn't see that ngModel optional requirement make it in though. Is that ok?

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Feb 18, 2015

Member

Oops

On 18 February 2015 at 13:21, Stephen Barker notifications@github.com
wrote:

Thanks @petebacondarwin https://github.com/petebacondarwin! I didn't
see that ngModel optional requirement make it in though. Is that ok?


Reply to this email directly or view it on GitHub
#11017 (comment).

Member

petebacondarwin commented Feb 18, 2015

Oops

On 18 February 2015 at 13:21, Stephen Barker notifications@github.com
wrote:

Thanks @petebacondarwin https://github.com/petebacondarwin! I didn't
see that ngModel optional requirement make it in though. Is that ok?


Reply to this email directly or view it on GitHub
#11017 (comment).

sjbarker added a commit to sjbarker/angular.js that referenced this pull request Feb 18, 2015

feat(ngOptions): add support for disabling an option
This patch adds support for disabling options based on model values. The
"disable when" syntax allows for listening to changes on those model values,
in order to dynamically enable and disable the options.

The changes prevent disabled options from being written to the selectCtrl
from the model. If a disabled selection is present on the model, normal
unknown or empty functionality kicks in.

Closes #638
Closes #11017
@sjbarker

This comment has been minimized.

Show comment
Hide comment
@sjbarker

sjbarker Feb 18, 2015

Contributor

@petebacondarwin - f3ae91d here has the commit you just merged with the ngModel require change.

Contributor

sjbarker commented Feb 18, 2015

@petebacondarwin - f3ae91d here has the commit you just merged with the ngModel require change.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Feb 18, 2015

Member

Thanks - I fixed that here: ef894c8

Member

petebacondarwin commented Feb 18, 2015

Thanks - I fixed that here: ef894c8

@sjbarker

This comment has been minimized.

Show comment
Hide comment
@sjbarker

sjbarker Feb 18, 2015

Contributor

Ok perfect

Contributor

sjbarker commented Feb 18, 2015

Ok perfect

@IngoVals

This comment has been minimized.

Show comment
Hide comment
@IngoVals

IngoVals Jun 15, 2015

If the disabling requirements are dynamic and something that was selected becomes disable the value is set to null, could we intercept this event and change it to something else?

IngoVals commented Jun 15, 2015

If the disabling requirements are dynamic and something that was selected becomes disable the value is set to null, could we intercept this event and change it to something else?

@sjbarker

This comment has been minimized.

Show comment
Hide comment
@sjbarker

sjbarker Jun 18, 2015

Contributor

@IngoVals when you say change it to something else, what do you mean?

Contributor

sjbarker commented Jun 18, 2015

@IngoVals when you say change it to something else, what do you mean?

netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015

feat(ngOptions): add support for disabling an option
This patch adds support for disabling options based on model values. The
"disable when" syntax allows for listening to changes on those model values,
in order to dynamically enable and disable the options.

The changes prevent disabled options from being written to the selectCtrl
from the model. If a disabled selection is present on the model, normal
unknown or empty functionality kicks in.

Closes #638
Closes #11017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment