Skip to content
This repository was archived by the owner on Jan 6, 2025. It is now read-only.

fix(api, class): remove ‘[class]’ selector #394

Merged
merged 1 commit into from
Sep 7, 2017
Merged

fix(api, class): remove ‘[class]’ selector #394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/lib/api/core/base-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ export class BaseFxDirectiveAdapter extends BaseFxDirective {
} else if (typeof source === 'string') {
this._cacheInputString(key, source);
} else {
throw new Error('Invalid class value provided. Did you want to cache the raw value?');
throw new Error(
`Invalid class value '${key}' provided. Did you want to cache the raw value?`
);
}
}

Expand Down Expand Up @@ -124,4 +126,14 @@ export class BaseFxDirectiveAdapter extends BaseFxDirective {
protected _cacheInputString(key = '', source?: string) {
this._inputMap[key] = source;
}

/**
* Does this directive have 1 or more responsive keys defined
* Note: we exclude the 'baseKey' key (which is NOT considered responsive)
*/
public get usesResponsiveAPI() {
Copy link
Member

Choose a reason for hiding this comment

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

Something like this might be more concise:

public get usesResponsiveAPI() {
  const numKeys = Object.keys(this._inputMap).length;
  return !!(numKeys === 1 || !this._inputMap[this._baseKey]);
}

Copy link
Contributor Author

@ThomasBurleson ThomasBurleson Sep 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

const totalKeys = Object.keys(this._inputMap).length;
const baseValue = this._inputMap[this._baseKey];
return (totalKeys - (!!baseValue ? 1 : 0)) > 0;
}
}
37 changes: 30 additions & 7 deletions src/lib/api/ext/class.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,21 +65,44 @@ describe('class directive', () => {
});

it('should override base `class` values with responsive values', () => {
createTestComponent(`<div class="class0" class.xs="class1 class2" ngClass.xs="what"></div>`);
createTestComponent(`<div class="class0" ngClass.xs="what class2"></div>`);

expectNativeEl(fixture).toHaveCssClass('class0');
expectNativeEl(fixture).not.toHaveCssClass('class1');
expectNativeEl(fixture).not.toHaveCssClass('what');
expectNativeEl(fixture).not.toHaveCssClass('class2');

// the CSS classes listed in the string (space delimited) are added,
matchMedia.activate('xs');
expectNativeEl(fixture).toHaveCssClass('class0');
expectNativeEl(fixture).toHaveCssClass('what');
expectNativeEl(fixture).toHaveCssClass('class2');

matchMedia.activate('lg');
expectNativeEl(fixture).toHaveCssClass('class0');
expectNativeEl(fixture).not.toHaveCssClass('what');
expectNativeEl(fixture).not.toHaveCssClass('class2');
});

it('should override base `class` values with responsive values', () => {
createTestComponent(`
<div class="class0" [ngClass.xs]="{'what':true, 'class2':true, 'class0':false}"></div>
`);

expectNativeEl(fixture).toHaveCssClass('class0');
expectNativeEl(fixture).not.toHaveCssClass('what');
expectNativeEl(fixture).not.toHaveCssClass('class2');

// Object keys are CSS classes that get added when the expression given in
// the value evaluates to a truthy value, otherwise they are removed.
matchMedia.activate('xs');
expectNativeEl(fixture).not.toHaveCssClass('class0');
expectNativeEl(fixture).toHaveCssClass('class1');
expectNativeEl(fixture).toHaveCssClass('what');
expectNativeEl(fixture).toHaveCssClass('class2');

// matchMedia.activate('lg');
// expectNativeEl(fixture).toHaveCssClass('class0');
// expectNativeEl(fixture).not.toHaveCssClass('class1');
// expectNativeEl(fixture).not.toHaveCssClass('class2');
matchMedia.activate('lg');
expectNativeEl(fixture).toHaveCssClass('class0');
expectNativeEl(fixture).not.toHaveCssClass('what');
expectNativeEl(fixture).not.toHaveCssClass('class2');
});

it('should keep the raw existing `class` with responsive updates', () => {
Expand Down
64 changes: 24 additions & 40 deletions src/lib/api/ext/class.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export type NgClassType = string | string[] | Set<string> | {[klass: string]: an
*/
@Directive({
selector: `
[class], [class.xs], [class.sm], [class.md], [class.lg], [class.xl],
[class.xs], [class.sm], [class.md], [class.lg], [class.xl],
[class.lt-sm], [class.lt-md], [class.lt-lg], [class.lt-xl],
[class.gt-xs], [class.gt-sm], [class.gt-md], [class.gt-lg],

Expand Down Expand Up @@ -77,41 +77,21 @@ export class ClassDirective extends BaseFxDirective

/** Deprecated selectors */

/**
* Base class selector values get applied immediately and are considered destructive overwrites to
* all previous class assignments
*
* Delegate to NgClass:klass setter and cache value for base fallback from responsive APIs.
*/
@Input('class')
set classBase(val: string) {
this._classAdapter.cacheInput('_rawClass', val, true);
this._ngClassInstance.klass = val;
}
@Input('class.xs') set classXs(val: NgClassType) { this._classAdapter.cacheInput('classXs', val); }
@Input('class.sm') set classSm(val: NgClassType) { this._classAdapter.cacheInput('classSm', val); }
@Input('class.md') set classMd(val: NgClassType) { this._classAdapter.cacheInput('classMd', val); }
@Input('class.lg') set classLg(val: NgClassType) { this._classAdapter.cacheInput('classLg', val); }
@Input('class.xl') set classXl(val: NgClassType) { this._classAdapter.cacheInput('classXl', val); }

@Input('class.xs') set classXs(val: NgClassType) { this._classAdapter.cacheInput('classXs', val, true); }
@Input('class.sm') set classSm(val: NgClassType) { this._classAdapter.cacheInput('classSm', val, true); }
@Input('class.md') set classMd(val: NgClassType) { this._classAdapter.cacheInput('classMd', val, true); }
@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-sm') set classLtSm(val: NgClassType) { this._classAdapter.cacheInput('classLtSm', val); }
@Input('class.lt-md') set classLtMd(val: NgClassType) { this._classAdapter.cacheInput('classLtMd', val); }
@Input('class.lt-lg') set classLtLg(val: NgClassType) { this._classAdapter.cacheInput('classLtLg', val); }
@Input('class.lt-xl') set classLtXl(val: NgClassType) { this._classAdapter.cacheInput('classLtXl', val); }

@Input('class.lt-sm') set classLtSm(val: NgClassType) { this._classAdapter.cacheInput('classLtSm', val, true); }
@Input('class.lt-md') set classLtMd(val: NgClassType) { this._classAdapter.cacheInput('classLtMd', val, true); }
@Input('class.lt-lg') set classLtLg(val: NgClassType) { this._classAdapter.cacheInput('classLtLg', val, true); }
@Input('class.lt-xl') set classLtXl(val: NgClassType) { this._classAdapter.cacheInput('classLtXl', val, true); }

@Input('class.gt-xs') set classGtXs(val: NgClassType) { this._classAdapter.cacheInput('classGtXs', val, true); }
@Input('class.gt-sm') set classGtSm(val: NgClassType) { this._classAdapter.cacheInput('classGtSm', val, true); }
@Input('class.gt-md') set classGtMd(val: NgClassType) { this._classAdapter.cacheInput('classGtMd', val, true); }
@Input('class.gt-lg') set classGtLg(val: NgClassType) { this._classAdapter.cacheInput('classGtLg', val, true); }

/**
* Initial value of the `class` attribute; used as
* fallback and will be merged with nay `ngClass` values
*/
get initialClasses() : string {
return this._classAdapter.queryInput('_rawClass') || "";
}
@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); }
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

@crisbeto was this addressed before merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crisbeto - there is a PR for this ^ feature in @angular: angular/angular#13355


/* tslint:enable */
constructor(protected monitor: MediaMonitor,
Expand All @@ -121,8 +101,9 @@ export class ClassDirective extends BaseFxDirective

super(monitor, _ngEl, _renderer);

this._classAdapter = new BaseFxDirectiveAdapter('class', monitor, _ngEl, _renderer);
this._ngClassAdapter = new BaseFxDirectiveAdapter('ngClass', monitor, _ngEl, _renderer);
this._classAdapter = new BaseFxDirectiveAdapter('class', monitor, _ngEl, _renderer);
this._classAdapter.cacheInput('class', _ngEl.nativeElement.getAttribute('class') || '');

// Create an instance NgClass Directive instance only if `ngClass=""` has NOT been defined on
// the same host element; since the responsive variations may be defined...
Expand Down Expand Up @@ -156,7 +137,9 @@ export class ClassDirective extends BaseFxDirective
if (!this._classAdapter.hasMediaQueryListener) {
this._configureMQListener();
}
this._ngClassInstance.ngDoCheck();
if ( this._ngClassInstance) {
this._ngClassInstance.ngDoCheck();
}
}

ngOnDestroy() {
Expand All @@ -174,11 +157,12 @@ export class ClassDirective extends BaseFxDirective
* mql change events to onMediaQueryChange handlers
*/
protected _configureMQListener() {
this._classAdapter.listenForMediaQueryChanges('class', '', (changes: MediaChange) => {
const value = this._classAdapter.queryInput('class');
this._classAdapter.listenForMediaQueryChanges('class', value, (changes: MediaChange) => {
this._updateKlass(changes.value);
});

this._ngClassAdapter.listenForMediaQueryChanges('ngClass', '', (changes: MediaChange) => {
this._ngClassAdapter.listenForMediaQueryChanges('ngClass', value, (changes: MediaChange) => {
this._updateNgClass(changes.value);
this._ngClassInstance.ngDoCheck(); // trigger NgClass::_applyIterableChanges()
});
Expand All @@ -189,11 +173,11 @@ export class ClassDirective extends BaseFxDirective
* ::ngDoCheck() is not needed
*/
protected _updateKlass(value?: NgClassType) {
let klass = value || this._classAdapter.queryInput('class') || '';
let klass = value || this._classAdapter.queryInput('class');
if (this._classAdapter.mqActivation) {
klass = this._classAdapter.mqActivation.activatedInput;
}
this._ngClassInstance.klass = klass || this.initialClasses;
this._ngClassInstance.klass = klass;
}

/**
Expand Down