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

feat(ngModel): provide ng-empty and ng-not-empty CSS classes#12848

Merged
petebacondarwin merged 2 commits intoangular:masterfrom
petebacondarwin:issue-10050
Sep 22, 2015
Merged

feat(ngModel): provide ng-empty and ng-not-empty CSS classes#12848
petebacondarwin merged 2 commits intoangular:masterfrom
petebacondarwin:issue-10050

Conversation

@petebacondarwin
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin commented Sep 14, 2015

I believe that this feature should be enough to fix #10050

@petebacondarwin petebacondarwin added this to the 1.5.x - migration-facilitation milestone Sep 14, 2015
@gkalpak
Copy link
Copy Markdown
Member

gkalpak commented Sep 14, 2015

I wonder if people will ask for this to be propagated to a parent FormController...

@petebacondarwin
Copy link
Copy Markdown
Contributor Author

petebacondarwin commented Sep 14, 2015

I was trying not to mention that :-P

@Narretz
Copy link
Copy Markdown
Contributor

Narretz commented Sep 14, 2015

For the original use case, you would then useng-pristine and ng-not-empty together?

Narretz
Narretz reviewed Sep 14, 2015
View reviewed changes
src/ng/directive/ngModel.js Outdated
Copy link
Copy Markdown
Contributor

@Narretz Narretz Sep 14, 2015

Choose a reason for hiding this comment

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

Should this be ctrl?

Copy link
Copy Markdown
Contributor Author

@petebacondarwin petebacondarwin Sep 14, 2015

Choose a reason for hiding this comment

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

It doesn't really matter because we have control over when it is called but I agree it is probably better for consistency.

@petebacondarwin
Copy link
Copy Markdown
Contributor Author

petebacondarwin commented Sep 14, 2015

Thanks for the reviews @Narretz and @gkalpak. I added a commit.

@petebacondarwin
Copy link
Copy Markdown
Contributor Author

petebacondarwin commented Sep 21, 2015

@Narretz and @gkalpak can you take another look?

Narretz
Narretz reviewed Sep 21, 2015
View reviewed changes
src/ng/directive/ngModel.js
Copy link
Copy Markdown
Contributor

@Narretz Narretz Sep 21, 2015

Choose a reason for hiding this comment

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

This could be simplified by always using ctrl.$viewValue instead of passing it as an arg. It should only be the viewValue anyway. You'll have to change the order on line https://github.com/angular/angular.js/pull/12848/files#diff-ae1963e24d6328c3aa019c0c1b9f4432R851 though, because otherwise it'll use the old viewValue

Copy link
Copy Markdown
Contributor Author

@petebacondarwin petebacondarwin Sep 21, 2015

Choose a reason for hiding this comment

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

Actually it would have to use $$lastCommittedViewValue and we would have to change the order as you suggest. I am not that keen on doing this though as I don't feel that not passing in an argument makes it that much simpler.

@Narretz
Copy link
Copy Markdown
Contributor

Narretz commented Sep 21, 2015

Regarding my question

For the original use case, you would then use ng-pristine and ng-not-empty together?

because only ng-empty doesn't work if I understood the issue correctly.

@petebacondarwin
Copy link
Copy Markdown
Contributor Author

petebacondarwin commented Sep 21, 2015

@Narretz: Yes, you would use a combination of the CSS classes for the original use case, but this seemed reasonable and intuitive to me.

@petebacondarwin
Copy link
Copy Markdown
Contributor Author

petebacondarwin commented Sep 21, 2015

Thanks for the review @Narretz.
So assuming that we don't simplify the method and that the original use case is covered by a combination of CSS classes. Is this PR ready to merge?

@Narretz
Copy link
Copy Markdown
Contributor

Narretz commented Sep 21, 2015

Yep, feel free to merge!
Am 21.09.2015 22:45 schrieb "Pete Bacon Darwin" notifications@github.com:

Thanks for the review @Narretz https://github.com/Narretz.
So assuming that we don't simplify the method and that the original use
case is covered by a combination of CSS classes. Is this PR ready to merge?

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

@petebacondarwin
test(ngModel): remove jankiness test
496a67c 
It is no longer appropriate to test this here as $animate takes care of
queueing up CSS class changes into a single update.
@petebacondarwin
feat(ngModel): provide ng-empty and ng-not-empty CSS classes
630280c 
If the `$viewValue` is empty then the `ng-empty` CSS class is applied
to the input. Conversely, if it is not empty the `ng-not-empty` CSS class
is applied. Emptiness is ascertained by calling `NgModelController.$isEmpty()`

Closes angular#10050
Closes angular#12848
@petebacondarwin petebacondarwin merged commit 630280c into angular:master Sep 22, 2015
@petebacondarwin petebacondarwin deleted the issue-10050 branch November 24, 2016 09:05
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 component: forms frequency: low needs: public api needs: review severity: inconvenient type: feature

Projects

None yet

Milestone

1.5.x

Development

Successfully merging this pull request may close these issues.

Proposal: .ng-prefilled class helper

3 participants

@petebacondarwin @gkalpak @Narretz
X Tutup