fix(api, class): remove ‘[class]’ selector #394
Conversation
ThomasBurleson
force-pushed
the
thomas/issue-393
branch
from
ed807e3
to
b399886
Compare
ThomasBurleson
force-pushed
the
thomas/issue-393
branch
3 times, most recently
from
06eea6c
to
c7f0f47
Compare
|
@crisbeto - would you mind reviewing this one ? |
| * Does this directive have 1 or more responsive keys defined | ||
| * Note: we exclude the 'baseKey' key (which is NOT considered responsive) | ||
| */ | ||
| public get usesResponsiveAPI() { |
There was a problem hiding this comment.
Something like this might be more concise:
public get usesResponsiveAPI() {
const numKeys = Object.keys(this._inputMap).length;
return !!(numKeys === 1 || !this._inputMap[this._baseKey]);
}There was a problem hiding this comment.
@crisbeto - that logic is not correct since we are checking for 1 or more non-baseKey values.
public get usesResponsiveAPI() {
const totalKeys = Object.keys(this._inputMap).length;
const baseValue = this._inputMap[this._baseKey];
return (totalKeys - (!!baseValue ? 1 : 0)) > 0;
}
src/lib/api/core/base.ts
Outdated
| @@ -130,7 +134,7 @@ export abstract class BaseFxDirective implements OnDestroy, OnChanges { | |||
| * and optional restore it when the mediaQueries deactivate | |||
| */ | |||
| protected _getDisplayStyle(source?: HTMLElement): string { | |||
| let element: HTMLElement = source || this._elementRef.nativeElement; | |||
| let element: HTMLElement = source || this.nativeElement; | |||
There was a problem hiding this comment.
Consider switching the method signature to source = this.nativeElement here and in _applyStyleToElement.
| @Input('class.gt-xs') set classGtXs(val: NgClassType) { this._classAdapter.cacheInput('classGtXs', val); } | ||
| @Input('class.gt-sm') set classGtSm(val: NgClassType) { this._classAdapter.cacheInput('classGtSm', val); } | ||
| @Input('class.gt-md') set classGtMd(val: NgClassType) { this._classAdapter.cacheInput('classGtMd', val); } | ||
| @Input('class.gt-lg') set classGtLg(val: NgClassType) { this._classAdapter.cacheInput('classGtLg', val); } |
There was a problem hiding this comment.
All of these setters will generate a lot of ES5 JS. Have you considered using ngOnChanges and looping through the changed inputs to cache them instead?
There was a problem hiding this comment.
@crisbeto was this addressed before merge?
There was a problem hiding this comment.
@crisbeto - there is a PR for this ^ feature in @angular: angular/angular#13355
src/lib/api/flexbox/layout-wrap.ts
Outdated
| @@ -142,7 +142,7 @@ export class LayoutWrapDirective extends BaseFxDirective implements OnInit, OnCh | |||
| } | |||
|
|
|||
| protected get flowDirection(): string { | |||
| let computeFlowDirection = () => this._getFlowDirection(this._elementRef.nativeElement); | |||
| let computeFlowDirection = () => this._getFlowDirection(this.nativeElement); | |||
| return this._layoutWatcher ? this._layout : computeFlowDirection(); | |||
There was a problem hiding this comment.
Defining a function just to call it immediately seems a little wasteful. Consider returning this._getFlowDirection(this.nativeElement) instead?
crisbeto
added
pr: lgtm
ThomasBurleson
force-pushed
the
thomas/issue-393
branch
from
c7f0f47
to
9c1c8d9
Compare
The host `class` attribute should be considered static and should not be used as a ClassDirective selector. This means that without associated responsive APIs, the ClassDirective will not be instantiated for elements using only the `class` attribute. * Use the `class` attribute as a responsive fallback value only. * Improve support of the ngClass directive for string and object assignments. Fixes #393.
ThomasBurleson
force-pushed
the
thomas/issue-393
branch
from
9c1c8d9
to
8352254
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
The host
classattribute should be considered static and should not be used as a ClassDirective selector. This means that without associated responsive APIs, the ClassDirective will not be instantiated for elements using only theclassattribute.classattribute as a responsive fallback value only.Fixes #393.