fix(ngClass,ngStyle): support proper API usages and ChangeDetectionStrategy.OnPush strategies #228

ThomasBurleson · 2017-03-19T15:24:26Z

fix(ngClass): properly differentiate between ngClass and class api usages
- class.<xxx> are destructive overrides (as seen in NgClass::klass)
- ngClass.<xxx> use iterables to merge/enable/disable 0...n classes
fix(ngClass,ngStyle): support ChangeDetectionStrategy.OnPush
- Add support for both OnPush change detection strategies and for @input changes.

@see https://plnkr.co/edit/jCICQ1GXFnzFqFTFlXwh?p=preview

fixes #206, fixes #215.

jelbourn · 2017-03-20T17:20:16Z

src/lib/flexbox/api/base-adapter.ts


 /**
 * Adapter to the BaseFxDirective abstract class so it can be used via composition.
 * @see BaseFxDirective
 */
 export class BaseFxDirectiveAdapter extends BaseFxDirective {
+
+  get activeKey() {


This needs a description comment

jelbourn · 2017-03-20T17:20:53Z

src/lib/flexbox/api/base-adapter.ts

@@ -19,10 +38,21 @@ export class BaseFxDirectiveAdapter extends BaseFxDirective {
  }

  /**
+     *
+     */


Empty comment block?

jelbourn · 2017-03-20T17:22:03Z

src/lib/flexbox/api/base-adapter.ts

@@ -19,10 +38,21 @@ export class BaseFxDirectiveAdapter extends BaseFxDirective {
  }

  /**
+     *
+     */
+  constructor(protected _baseKey: string,


baseKey needs a description comment (as a class member) since it's non-obvious

jelbourn · 2017-03-20T17:23:32Z

src/lib/flexbox/api/base.ts

-   *  Dictionary of input keys with associated values
-   */
-  protected _inputMap = {};
+  get hasMQListener() {


hasMediaQueryListener?

jelbourn · 2017-03-20T17:24:29Z

src/lib/flexbox/api/base.ts

+      this._mqActivation = new ResponsiveActivation(
+          keyOptions,
+          this._mediaMonitor,
+          (change) => onMediaQueryChange.call(this, change)


(change) => this.onMediaQueryChange(change)

(there's no need for .call here)

jelbourn · 2017-03-20T17:25:16Z

src/lib/flexbox/api/base.ts

@@ -201,4 +195,18 @@ export abstract class BaseFxDirective implements OnDestroy {
    return this._mqActivation.hasKeyValue(key);
  }

+  /**
+   * Original dom Elements CSS display style
+   */


You can collapse short description blocks into a single line:

/** The element's original CSS `display` property value. */

jelbourn · 2017-03-20T17:27:14Z

src/lib/flexbox/api/class.ts

              _iterableDiffers: IterableDiffers, _keyValueDiffers: KeyValueDiffers,
              _ngEl: ElementRef, _renderer: Renderer) {
    super(_iterableDiffers, _keyValueDiffers, _ngEl, _renderer);
-    this._base = new BaseFxDirectiveAdapter(monitor, _ngEl, _renderer);
+    this._base = new BaseFxDirectiveAdapter("class", monitor, _ngEl, _renderer);


Single quotes (there should be a lint check for this)

jelbourn · 2017-03-20T17:27:51Z

src/lib/flexbox/api/class.ts

@@ -72,7 +69,7 @@ export class ClassDirective extends NgClass implements OnInit, OnChanges, OnDest
  @Input('ngClass.gt-lg') set ngClassGtLg(val: NgClassType) { this._base.cacheInput('classGtLg', val, true); };

  /** Deprecated selectors */
-  @Input('class')         set classBase(val: NgClassType) { this._base.cacheInput('class', val, true); }
+  @Input('class')         set klazz(val: NgClassType) { this._base.cacheInput('class', val, true); }


cssClass would be a lot more descriptive than klazz

This is a similar convention used by NgClass:

@Input('class') set klass(v: string) { ... }

jelbourn · 2017-03-20T17:29:43Z

src/lib/flexbox/api/class.ts

-    if (changed || this._base.mqActivation) {
-      this._updateClass();
+    const changed = (this._base.activeKey in changes);
+    return changed && this._updateClass();


Angular does not do anything with the return value of lifecycle events

ThomasBurleson · 2017-03-20T20:52:45Z

@jelbourn - comments addressed. Ready for presubmit.

ThomasBurleson · 2017-03-22T11:08:17Z

@jelbourn - I will remove support for class.<xxx> and style.<xxx> responsive APIs and require developers to use ngClass.<xxx> and ngStyle.<xxx> APIs.

ThomasBurleson · 2017-03-27T21:14:27Z

@jelbourn - ready for review and presubmit. All tests pass.

Current tests include cloned copies of those from the Angular core ng-class-spec.ts tests.

ThomasBurleson · 2017-03-27T21:25:14Z

@mmalerba - can you review these fixes ? Thanks.

mmalerba · 2017-03-28T16:40:22Z

src/lib/flexbox/api/class.ts

+  @Input('ngClass.lg')    set ngClassLg(val: NgClassType) { this._ngClassAdapter.cacheInput('ngClassLg', val, true);};
+  @Input('ngClass.xl')    set ngClassXl(val: NgClassType) { this._ngClassAdapter.cacheInput('ngClassXl', val, true); };
+
+  @Input('ngClass.lt-xs') set ngClassLtXs(val: NgClassType) { this._ngClassAdapter.cacheInput('ngClassLtXs', val, true); };


Is lt-xs a real thing? I think this should be lt-xl

mmalerba · 2017-03-28T16:47:09Z

src/lib/flexbox/api/base-adapter.ts

+   * e.g. which property value will be used.
+   */
+  get activeKey() {
+    let mqa = this._mqActivation;


this seems pointless? just use _mqActivation below

this is a short-cut reference to keep the code clean.

mmalerba · 2017-03-28T16:47:14Z

src/lib/flexbox/api/base-adapter.ts

+    let key = mqa ? mqa.activatedInputKey : this._baseKey;
+    // ClassDirective::SimpleChanges uses 'klazz'
+    // instead of 'class' as a key
+    return (key === 'class') ? 'klazz' : key;


Just want to double check, this is really what you want right? I see klazz, clazz and klass at various points in the code. Also future work: we should make this consistent everywhere, I vote for cssClass

angular ng-class.ts uses klass. Since they already set a precedent, why not be consistent?

Ok but is this the correct one? because I do see all three of these in the code. More important than what we decide to call it is that it be consistent within our code instead of having 3 different variants. But I don't think we necessarily have to follow the same naming conventions for internal variables as them, this is a totally separate repo.

Oh ok I see this is referencing the one from angular core, no issue for this PR then, just the naming thing to consider in the future

mmalerba · 2017-03-28T16:49:36Z

src/lib/flexbox/api/class.ts

+  @Input('class.lg')      set classLg(val: NgClassType) { this._classAdapter.cacheInput('classLg', val, true); };
+  @Input('class.xl')      set classXl(val: NgClassType) { this._classAdapter.cacheInput('classXl', val, true); };
+
+  @Input('class.lt-xs')   set classLtXs(val: NgClassType) { this._classAdapter.cacheInput('classLtXs', val, true); };


should this be lt-xl?

lt-xl, lt-lg, lt-md,lt-sm are all valid responsive aliases.

I see... the Inputs are not entirely correct. I will fix.

@input

…rategy.OnPush strategies * fix(ngClass): properly differentiate between `ngClass` and `class` api usages * `class.<xxx>` are destructive overrides (as seen in `NgClass::klass`) * `ngClass.<xxx>` use iterables to merge/enable/disable 0...n classes * fix(ngClass,ngStyle): support ChangeDetectionStrategy.OnPush * Add support for both **OnPush** change detection strategies and for @input changes. * fix class lt-<xx> selectors > @see https://plnkr.co/edit/jCICQ1GXFnzFqFTFlXwh?p=preview fixes #206, fixes #215.

ThomasBurleson · 2017-03-28T20:51:16Z

FYI - A design doc is being prepared to discuss the Responsive API for ngClass, class, ngStyle, and style. Future changes - based on the document - may be presented via 1 or more additional PRs.

See more details here: angular/flex-layout#228

@input

…rategy.OnPush strategies (angular#228) * fix(ngClass): properly differentiate between `ngClass` and `class` api usages * `class.<xxx>` are destructive overrides (as seen in `NgClass::klass`) * `ngClass.<xxx>` use iterables to merge/enable/disable 0...n classes * fix(ngClass,ngStyle): support ChangeDetectionStrategy.OnPush * Add support for both **OnPush** change detection strategies and for @input changes. * fix class lt-<xx> selectors > @see https://plnkr.co/edit/jCICQ1GXFnzFqFTFlXwh?p=preview fixes angular#206, fixes angular#215.

angular-automatic-lock-bot · 2019-09-05T22:03:21Z

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 Mar 19, 2017

ThomasBurleson force-pushed the thomas/fix-class-onPush branch 5 times, most recently from c0cb733 to cf60c0a Compare March 19, 2017 20:28

jelbourn reviewed Mar 20, 2017

View reviewed changes

ThomasBurleson force-pushed the thomas/fix-class-onPush branch 2 times, most recently from 2c67ed4 to 40a33b1 Compare March 20, 2017 20:52

ThomasBurleson added the pr: needs presubmit label Mar 20, 2017

ThomasBurleson added this to the v2.0.0-beta.7 milestone Mar 20, 2017

ThomasBurleson force-pushed the thomas/fix-class-onPush branch 2 times, most recently from 95bb161 to d2517b8 Compare March 22, 2017 01:52

ThomasBurleson force-pushed the thomas/fix-class-onPush branch from d2517b8 to 72afd0c Compare March 27, 2017 16:16

ThomasBurleson changed the title ~~fix(ngClass,ngStyle): support ChangeDetectionStrategy.OnPush~~ fix(ngClass,ngStyle): support proper API usages and ChangeDetectionStrategy.OnPush strategies Mar 27, 2017

ThomasBurleson force-pushed the thomas/fix-class-onPush branch 2 times, most recently from 167cb8c to 9c425aa Compare March 27, 2017 20:53

ThomasBurleson mentioned this pull request Mar 27, 2017

API Documentation for ngStyle and ngClass uses different syntax #215

Closed

ThomasBurleson force-pushed the thomas/fix-class-onPush branch from 9c425aa to 7bb2d96 Compare March 27, 2017 21:17

mmalerba self-requested a review March 27, 2017 22:37

mmalerba self-assigned this Mar 27, 2017

ThomasBurleson force-pushed the thomas/fix-class-onPush branch from 7bb2d96 to 1bf533b Compare March 28, 2017 03:28

mmalerba requested changes Mar 28, 2017

View reviewed changes

mmalerba removed the pr: needs presubmit label Mar 28, 2017

ThomasBurleson force-pushed the thomas/fix-class-onPush branch from 1bf533b to 446fc8b Compare March 28, 2017 17:38

ThomasBurleson force-pushed the thomas/fix-class-onPush branch from 446fc8b to 5c098d6 Compare March 28, 2017 17:59

mmalerba approved these changes Mar 28, 2017

View reviewed changes

mmalerba added the pr: lgtm This PR has been approved by the reviewer label Mar 28, 2017

ThomasBurleson added the pr: needs presubmit label Mar 28, 2017

tinayuangao merged commit 5db01e7 into master Mar 29, 2017

ThomasBurleson deleted the thomas/fix-class-onPush branch March 30, 2017 23:28

felixjung pushed a commit to felixjung/tinydraft that referenced this pull request Apr 30, 2017

Workaround for a bug in flexLayout until fix hits npm

228a715

See more details here: angular/flex-layout#228

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngClass,ngStyle): support proper API usages and ChangeDetectionStrategy.OnPush strategies #228

fix(ngClass,ngStyle): support proper API usages and ChangeDetectionStrategy.OnPush strategies #228

ThomasBurleson commented Mar 19, 2017 •

edited

jelbourn Mar 20, 2017

jelbourn Mar 20, 2017

jelbourn Mar 20, 2017

jelbourn Mar 20, 2017

jelbourn Mar 20, 2017

jelbourn Mar 20, 2017

jelbourn Mar 20, 2017

jelbourn Mar 20, 2017

ThomasBurleson Mar 20, 2017

jelbourn Mar 20, 2017

ThomasBurleson commented Mar 20, 2017

ThomasBurleson commented Mar 22, 2017 •

edited

ThomasBurleson commented Mar 27, 2017 •

edited

ThomasBurleson commented Mar 27, 2017

mmalerba Mar 28, 2017

mmalerba Mar 28, 2017

ThomasBurleson Mar 28, 2017

mmalerba Mar 28, 2017

ThomasBurleson Mar 28, 2017

mmalerba Mar 28, 2017

mmalerba Mar 28, 2017

mmalerba Mar 28, 2017

ThomasBurleson Mar 28, 2017

ThomasBurleson Mar 28, 2017

ThomasBurleson commented Mar 28, 2017

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

                 }
                 /**
+                   *
+                   */

fix(ngClass,ngStyle): support proper API usages and ChangeDetectionStrategy.OnPush strategies #228

fix(ngClass,ngStyle): support proper API usages and ChangeDetectionStrategy.OnPush strategies #228

Conversation

ThomasBurleson commented Mar 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBurleson commented Mar 20, 2017

ThomasBurleson commented Mar 22, 2017 • edited

ThomasBurleson commented Mar 27, 2017 • edited

ThomasBurleson commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBurleson commented Mar 28, 2017

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

ThomasBurleson commented Mar 19, 2017 •

edited

ThomasBurleson commented Mar 22, 2017 •

edited

ThomasBurleson commented Mar 27, 2017 •

edited