Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(theming): log a warning if core theme isn't loaded #3781

Merged
merged 2 commits into from
Mar 29, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 5 additions & 0 deletions src/lib/core/_core.scss
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,9 @@
$background: map-get($theme, background);
background-color: mat-color($background, background);
}

// Marker that is used to determine whether the user has added a theme to their page.
.mat-theme-loaded-marker {
display: none;
}
}
33 changes: 31 additions & 2 deletions src/lib/core/compatibility/compatibility.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
} from '@angular/core';
import {DOCUMENT} from '@angular/platform-browser';

/** Flag for whether we've checked that the theme is loaded. */
let hasCheckedThemePresence = false;

export const MATERIAL_COMPATIBILITY_MODE = new OpaqueToken('md-compatibility-mode');

Expand Down Expand Up @@ -170,14 +172,41 @@ export class CompatibilityModule {
};
}

constructor(@Optional() @Inject(DOCUMENT) document: any) {
if (isDevMode() && typeof document && !document.doctype) {
constructor(@Optional() @Inject(DOCUMENT) private _document: any) {
this._checkDoctype();
this._checkTheme();
}

private _checkDoctype(): void {
if (isDevMode() && this._document && !this._document.doctype) {
console.warn(
'Current document does not have a doctype. This may cause ' +
'some Angular Material components not to behave as expected.'
);
}
}

private _checkTheme(): void {
if (hasCheckedThemePresence || !this._document || !isDevMode()) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we move the isDevMode check to the constructor now that we're doing multiple checks in the compatibility module?

We can also just have one hasDoneGlobalChecks property for all checks instead of hasCheckedThemePresence.

return;
}

let testElement = this._document.createElement('div');
Copy link
Member

Choose a reason for hiding this comment

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

If you make the testElement position: absolute, you should remove all possibility of adding it causing a reflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

The test element is display: none already.


testElement.classList.add('mat-theme-loaded-marker');
this._document.body.appendChild(testElement);

if (getComputedStyle(testElement).display !== 'none') {
console.warn(
'Could not find Angular Material core theme. Most Material ' +
'components may not work as expected. For more info refer ' +
'to the theming guide: https://github.com/angular/material2/blob/master/guides/theming.md'
Copy link
Member

Choose a reason for hiding this comment

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

Can we link to material.angular.io instead?

);
}

this._document.body.removeChild(testElement);
hasCheckedThemePresence = true;
}
}


Expand Down