Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat(typeahead): apply angular watch to the typeahead-editable setting #4820

Closed
wants to merge 2 commits into from

Conversation

damonf
Copy link

@damonf damonf commented Nov 5, 2015

Adds a watch for the typeahead-editable attribute.

@icfantv
Copy link
Contributor

icfantv commented Nov 5, 2015

@damonf, what problem is this PR solving? Also, you're missing functional tests.

Please give this a read and in the future, it's generally a good idea to run an idea by the team first before spending time on a PR as we don't want you to possibly be wasting your time. Thanks.

@wesleycho
Copy link
Contributor

There was a prior issue reported by @damonf related to this in #4592.

@@ -44,6 +44,10 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.debounce', 'ui.bootstrap

//should it restrict model values to the ones selected from the popup only?
var isEditable = originalScope.$eval(attrs.typeaheadEditable) !== false;
originalScope.$watch(attrs.typeaheadEditable, function (newVal) {
isEditable = newVal !== false;
modelCtrl.$setValidity('editable', isEditable);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change, and editable state should not be related to ngModel's validity. This is a bad change.

@wesleycho
Copy link
Contributor

This needs tests.

The model $setValidity call needs to be removed as well.

@damonf
Copy link
Author

damonf commented Nov 5, 2015

Thanks for your patience.
So if i removed the $setValidity call and added some unit tests you would
accept the PR?

On Thu, Nov 5, 2015 at 5:54 PM, Wesley Cho [email protected] wrote:

This needs tests.

The model $setValidity call needs to be removed as well.


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

@wesleycho
Copy link
Contributor

I would be willing to accept this feature with the necessary changes.

Noting for reference, this would implement #2638.

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.

3 participants