feat(CSSClass): add support for string and array expresions by pkozlowski-opensource · Pull Request #2664 · angular/angular

pkozlowski-opensource · 2015-06-21T13:10:17Z

This adds support for lists and strings arguments to the [class] directive. I've got few improvements on my mind to the initial implementation, but at least this is the starting point that we can improve upon.

Now, I would like to argue that we should rename this class to NgClass and make it match the [ng-class] selector. Here are my arguments:

It reduces "magic" in the framework where the "class" is special in some kind. The entire system is easier to reason about when there are less special rules to follow
It reduces unnecessary work on "static" class attributes
It makes migration (both in terms of code and learning) easier

Closes #2025

vicb · 2015-06-22T07:37:31Z

modules/angular2/src/directives/class.ts

add return type : void everywhere it's applicable

OK, will do!

naomiblack · 2015-06-23T01:41:41Z

@vicb should this otherwise have the pr_state LGTM from you or is there more significant review and fixing needed?

vicb · 2015-06-23T06:12:36Z

modules/angular2/src/directives/class.ts

v.split(' ') (BTW you can now use /.../g - g flag is mandatory to transpile to Dart)

not adressed

@vicb I didn't change this one since I saw that you had some problems with regexp literals in #2695. I guess this is not a blocking point, right? But I would be more than happy to change this one as soon as we sure that this works both in JS and Dart.

I mean using String.split() here and get rid of the RegExp.

Oh, right. I see! Will do asap!

vicb · 2015-06-23T06:20:49Z

Added LGTM and more feedback

pkozlowski-opensource · 2015-06-23T13:25:36Z

I've cleaned up based on comments from @vicb (thnx for the review!). I'm not sure if anyone needs to review since it doesn't have "action: merge" label. I would love to have some feedback on this one from @vsavkin - especially the way I handle type switches.

pkozlowski-opensource · 2015-06-23T13:26:14Z

Adding to m.28 since the associated issue is in m.28

vicb · 2015-06-23T14:43:26Z

modules/angular2/src/directives/class.ts

diff.wrapped instanceof IterableChanges ?

Not sure...

Why ? the methods you call below expect that.

IMO it doesn't change much, I've got no objections changing this. Will do.

vicb · 2015-06-23T14:44:20Z

I have added more comments.

.split() has yet to be adressed

pkozlowski-opensource · 2015-06-24T10:02:37Z

@vicb see my comments above.

googlebot · 2015-06-24T10:02:39Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

Closes angular#2025

googlebot · 2015-06-24T12:32:37Z

CLAs look good, thanks!

pkozlowski-opensource · 2015-06-24T12:35:35Z

@vicb I've addressed all your latest comments apart from removing RegExpWrapper. Not sure what is going on and I've got very limited time / access to Inet atm. Could we land this one as-is (I would like to avoid further rebases) and remove the RegExpWrapper in a separate PR?

vicb · 2015-06-24T12:39:29Z

If this is ready to be merged I can fixup the commit to remove the RegExp before merging into master

pkozlowski-opensource · 2015-06-24T12:55:30Z

@vicb if you know how to remove RegExp, feel free to do so while merging. Would be much appreciated!

vicb · 2015-06-24T14:07:00Z

replaced by #2720

angular-automatic-lock-bot · 2019-09-06T16:23:04Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Jun 21, 2015

vicb reviewed Jun 22, 2015
View reviewed changes

naomiblack added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews state: PR and removed state: PR labels Jun 23, 2015

vicb reviewed Jun 23, 2015
View reviewed changes

vicb added the pr_state: LGTM label Jun 23, 2015

pkozlowski-opensource mentioned this pull request Jun 23, 2015

feat(NgStyle): add new NgStyle directive #2665

Closed

pkozlowski-opensource force-pushed the class_with_list branch from 96e5d9c to db7d5d4 Compare June 23, 2015 12:38

pkozlowski-opensource removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 23, 2015

pkozlowski-opensource added this to the alpha-28 milestone Jun 23, 2015

vicb reviewed Jun 23, 2015
View reviewed changes

dariajung force-pushed the master branch from 2c3c235 to 2932377 Compare June 23, 2015 20:50

googlebot added cla: no and removed cla: yes labels Jun 24, 2015

feat(CSSClass): add support for string and array expresions

2b93bcc

Closes angular#2025

pkozlowski-opensource force-pushed the class_with_list branch from db7d5d4 to 2b93bcc Compare June 24, 2015 12:32

googlebot added cla: yes and removed cla: no labels Jun 24, 2015

vicb mentioned this pull request Jun 24, 2015

feat(CSSClass): add support for string and array expresions #2720

Merged

vicb closed this Jun 24, 2015

pkozlowski-opensource deleted the class_with_list branch July 1, 2015 14:37

matanlurey mentioned this pull request Jul 8, 2015

Provide a way to bind class at runtime in template #2927

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Conversation

pkozlowski-opensource commented Jun 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naomiblack commented Jun 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb commented Jun 23, 2015

Uh oh!

pkozlowski-opensource commented Jun 23, 2015

Uh oh!

pkozlowski-opensource commented Jun 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb commented Jun 23, 2015

Uh oh!

pkozlowski-opensource commented Jun 24, 2015

Uh oh!

googlebot commented Jun 24, 2015

Uh oh!

googlebot commented Jun 24, 2015

Uh oh!

pkozlowski-opensource commented Jun 24, 2015

Uh oh!

vicb commented Jun 24, 2015

Uh oh!

pkozlowski-opensource commented Jun 24, 2015

Uh oh!

vicb commented Jun 24, 2015

Uh oh!

angular-automatic-lock-bot bot commented Sep 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants