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 #2846

Closed
wants to merge 2 commits into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jan 28, 2017

Checks the user's loaded stylesheets and logs a warning if the Material core theme isn't loaded.

Fixes #2828.

Note: I originally went with looping through the document.styleSheets to check whether the selector is defined, however I had to switch back to getComputedStyle, because browsers don't expose the document.styleSheets, if the CSS file is being loaded from another domain. This would've caused the warning to be logged if the user loads over a CDN.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 28, 2017
testElement.classList.add('md-theme-loaded-marker');
document.body.appendChild(testElement);

if (getComputedStyle(testElement).display !== 'none') {
Copy link
Member

Choose a reason for hiding this comment

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

What if instead we did something like

.mat-theme-load-test {
  width: 500px;
}

Then we create two elements (position absolute, min heights/width 1px), append them to body, and add the class to one of them then see if they have the same/different offsetWidth?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that it may end up being slower. Both getComputedStyle and offsetWidth cause a reflow, however we only need to call getComputedStyle once. As an aside, I tried doing some quick measurements between my initial approach (looping through the document.styleSheets) and the getComputedStyle approach. Both ended up taking about the same time on Chrome, but the latter was a bit faster in IE.

* @docs-private
*/
@NgModule()
export class MdThemeCheckModule {
Copy link
Member

Choose a reason for hiding this comment

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

What if we roll this functionality into CompatibilityModule so we don't have to add another module on every components?

Copy link
Member Author

@crisbeto crisbeto Jan 31, 2017

Choose a reason for hiding this comment

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

My reasoning for not adding it there is that, as far as I understand, it won't be a permanent module, but rather something that's there for a while until people have migrated completely to M2.

Checks the user's loaded stylesheets and logs a warning if the Material core theme isn't loaded.

Fixes angular#2828.
@crisbeto
Copy link
Member Author

crisbeto commented Mar 25, 2017

Closing this one and will re-submit something simpler that will do the check in the CompatibilityModule. Fixing all the merge conflicts here will take longer than redoing it from scratch.

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log a warning when a theme is not loaded
4 participants