Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

feat(themes): register theme on the fly #9413

Merged
merged 1 commit into from
Aug 31, 2016
Merged

feat(themes): register theme on the fly #9413

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
107 changes: 80 additions & 27 deletions src/core/services/theming/theming.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,20 +538,53 @@ function ThemingProvider($mdColorPalette, $$mdMetaProvider) {
* @param {el=} element to apply theming to
*/
/* @ngInject */
function ThemingService($rootScope, $log) {
function ThemingService($rootScope, $mdUtil, $q, $log) {
// Allow us to be invoked via a linking function signature.
var applyTheme = function (scope, el) {
if (el === undefined) { el = scope; scope = undefined; }
if (scope === undefined) { scope = $rootScope; }
applyTheme.inherit(el, el);
};

applyTheme.THEMES = angular.extend({}, THEMES);
applyTheme.PALETTES = angular.extend({}, PALETTES);
Object.defineProperty(applyTheme, 'THEMES', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this complexity... unless you want generate the THEMES object on demand (each time the getter is accessed).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, generating it once doesn't support themes that are registered on the fly

Copy link
Contributor

@hansl hansl Aug 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Reading this code, the following assertion happen, which seems like the wrong logic:

applyTheme.THEMES !== applyTheme.THEMES

And any app that changes THEMES (say by calling generateTheme) would not change references to THEMES that already exist.

get: function () {
return angular.extend({}, THEMES);
}
});
Object.defineProperty(applyTheme, 'PALETTES', {
get: function () {
return angular.extend({}, PALETTES);
}
});
applyTheme.inherit = inheritTheme;
applyTheme.registered = registered;
applyTheme.defaultTheme = function() { return defaultTheme; };
applyTheme.generateTheme = function(name) { generateTheme(THEMES[name], name, themeConfig.nonce); };
applyTheme.defineTheme = function(name, options) {
options = options || {};

var theme = registerTheme(name);

if (options.primary) {
theme.primaryPalette(options.primary);
}
if (options.accent) {
theme.accentPalette(options.accent);
}
if (options.warn) {
theme.warnPalette(options.warn);
}
if (options.background) {
theme.backgroundPalette(options.background);
}
if (options.dark){
theme.dark();
}

this.generateTheme(name);

return $q.resolve(name);
};
applyTheme.setBrowserColor = enableBrowserColor;

return applyTheme;
Expand All @@ -568,22 +601,31 @@ function ThemingProvider($mdColorPalette, $$mdMetaProvider) {
* Get theme name for the element, then update with Theme CSS class
*/
function inheritTheme (el, parent) {
var ctrl = parent.controller('mdTheme');
var attrThemeValue = el.attr('md-theme-watch');
var watchTheme = (alwaysWatchTheme || angular.isDefined(attrThemeValue)) && attrThemeValue != 'false';
var ctrl = parent.controller('mdTheme') || el.data('$mdThemeController');

updateThemeClass(lookupThemeName());

if ((alwaysWatchTheme && !registerChangeCallback()) || (!alwaysWatchTheme && watchTheme)) {
el.on('$destroy', $rootScope.$watch(lookupThemeName, updateThemeClass) );
if (ctrl) {
var watchTheme =
alwaysWatchTheme || ctrl.$shouldWatch || $mdUtil.parseAttributeBoolean(el.attr('md-theme-watch'));

var unwatch = ctrl.registerChanges(function (name) {
updateThemeClass(name);

if (!watchTheme) {
unwatch();
}
else {
el.on('$destroy', unwatch);
}
});
}

/**
* Find the theme name from the parent controller or element data
*/
function lookupThemeName() {
// As a few components (dialog) add their controllers later, we should also watch for a controller init.
ctrl = parent.controller('mdTheme') || el.data('$mdThemeController');
return ctrl && ctrl.$mdTheme || (defaultTheme == 'default' ? '' : defaultTheme);
}

Expand All @@ -606,24 +648,12 @@ function ThemingProvider($mdColorPalette, $$mdMetaProvider) {
el.data('$mdThemeController', ctrl);
}
}

/**
* Register change callback with parent mdTheme controller
*/
function registerChangeCallback() {
var parentController = parent.controller('mdTheme');
if (!parentController) return false;
el.on('$destroy', parentController.registerChanges( function() {
updateThemeClass(lookupThemeName());
}));
return true;
}
}

}
}

function ThemingDirective($mdTheming, $interpolate, $log) {
function ThemingDirective($mdTheming, $interpolate, $parse, $mdUtil, $q, $log) {
return {
priority: 100,
link: {
Expand All @@ -649,16 +679,39 @@ function ThemingDirective($mdTheming, $interpolate, $log) {
if (!$mdTheming.registered(theme)) {
$log.warn('attempted to use unregistered theme \'' + theme + '\'');
}


ctrl.$mdTheme = theme;

registeredCallbacks.forEach(function (cb) {
cb();
// Iterating backwards to support unregistering during iteration
// http://stackoverflow.com/a/9882349/890293
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that comment now invalid? since we are now using a simple reverse ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's still explaining why are we iterating backwards

Copy link
Member

@devversion devversion Aug 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The URL does in my case only shows how to loop backwards. But does not explain why we do this.

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 array is being re-indexed when you do a .splice(), which means you'll skip over an index when one is removed, and your cached .length is obsolete.

Copy link
Member

@devversion devversion Aug 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It's not a big deal. But where do we explicitly use a splice here? I can see no splice and .length` anymore :)

The comment is reasonable if we still use the snippet of that Stackoverflow, and it's still difficult enough to require some extra documentation.

Let's keep it as it is.

registeredCallbacks.reverse().forEach(function (cb) {
cb(theme);
});
}
},
$shouldWatch: $mdUtil.parseAttributeBoolean(el.attr('md-theme-watch'))
};

el.data('$mdThemeController', ctrl);
ctrl.$setTheme($interpolate(attrs.mdTheme)(scope));
attrs.$observe('mdTheme', ctrl.$setTheme);

var getThemeInterpolation = function () { return $interpolate(attrs.mdTheme)(scope); };

var setParsedTheme = function (interpolation) {
var theme = $parse(interpolation)(scope) || interpolation;

if (typeof theme === 'string') {
return ctrl.$setTheme(theme);
}

$q.when( (typeof theme === 'function') ? theme() : theme )
.then(function(name){
ctrl.$setTheme(name)
});
};

setParsedTheme(getThemeInterpolation());

scope.$watch(getThemeInterpolation, setParsedTheme);
}
}
};
Expand Down
68 changes: 52 additions & 16 deletions src/core/services/theming/theming.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,17 @@ describe('$mdTheming service', function() {
expect($mdTheming.defaultTheme()).toBe('default');
}));

it('supports registering theme on the fly', inject(function ($mdTheming) {
expect($mdTheming.THEMES.hasOwnProperty('test')).toBeFalsy();

$mdTheming.defineTheme('test', {
primary: 'red',
warn: 'yellow'
});

expect($mdTheming.THEMES.hasOwnProperty('test')).toBeTruthy();
}));

it('supports changing browser color on the fly', function() {
var name = 'theme-color';
var primaryPalette = $mdThemingProvider._THEMES.default.colors.primary.name;
Expand All @@ -758,7 +769,7 @@ describe('$mdTheming service', function() {
describe('md-theme directive', function() {
beforeEach(module('material.core'));

it('should observe and set mdTheme controller', inject(function($compile, $rootScope) {
it('should watch and set mdTheme controller', inject(function($compile, $rootScope) {
$rootScope.themey = 'red';
var el = $compile('<div md-theme="{{themey}}">')($rootScope);
$rootScope.$apply();
Expand All @@ -768,23 +779,48 @@ describe('md-theme directive', function() {
expect(ctrl.$mdTheme).toBe('blue');
}));

it('warns when an unregistered theme is used', function() {
inject(function($log, $compile, $rootScope) {
spyOn($log, 'warn');
$compile('<div md-theme="unregistered"></div>')($rootScope);
$rootScope.$apply();
expect($log.warn).toHaveBeenCalled();
});
});
it('warns when an unregistered theme is used', inject(function ($log, $compile, $rootScope) {
spyOn($log, 'warn');
$compile('<div md-theme="unregistered"></div>')($rootScope);
$rootScope.$apply();
expect($log.warn).toHaveBeenCalled();
}));

it('does not warn when a registered theme is used', inject(function($log, $compile, $rootScope) {
spyOn($log, 'warn');
$compile('<div md-theme="default"></div>')($rootScope);
$rootScope.$apply();
expect($log.warn.calls.count()).toBe(0);
}));

it('does not warn when a registered theme is used', function() {
inject(function($log, $compile, $rootScope) {
spyOn($log, 'warn');
$compile('<div md-theme="default"></div>')($rootScope);
it('should accept string as a theme', inject(function($compile, $rootScope, $mdTheming) {
var el = $compile('<div md-theme="red"></div>')($rootScope);
$rootScope.$apply();
$mdTheming(el);
expect(el.hasClass('md-default-theme')).toBeFalsy();
expect(el.hasClass('md-red-theme')).toBeTruthy();
}));

it('should accept $q promise as a theme', inject(function($compile, $rootScope, $q, $mdTheming) {
$rootScope.promise = $mdTheming.defineTheme('red', { primary: 'red' });
var el = $compile('<div md-theme="promise"></div>')($rootScope);
$mdTheming(el);
$rootScope.$apply();
expect(el.hasClass('md-default-theme')).toBeFalsy();
expect(el.hasClass('md-red-theme')).toBeTruthy();
}));

it('should accept a function that returns a promise as a theme',
inject(function ($compile, $rootScope, $q, $mdTheming) {
$rootScope.func = function () {
return $mdTheming.defineTheme('red', {primary: 'red'});
};
var el = $compile('<div md-theme="func"></div>')($rootScope);
$mdTheming(el);
$rootScope.$apply();
expect($log.warn.calls.count()).toBe(0);
});
});
expect(el.hasClass('md-default-theme')).toBeFalsy();
expect(el.hasClass('md-red-theme')).toBeTruthy();
}));
});

describe('md-themable directive', function() {
Expand Down