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

EladBezalel · 2016-08-26T01:45:39Z

Added defineTheme functionality to $mdTheming service
Added support for promise and a function that returns a promise on md-theme directive to support async build of themes

EladBezalel · 2016-08-26T20:38:36Z

@topherfangio we should find a better approach

ErinCoughlan · 2016-08-26T20:54:35Z

src/core/services/theming/theming.spec.js

+  it('does not warns when an unregistered theme is used', function() {
+    inject(function($log, $compile, $rootScope) {
+      spyOn($log, 'warn');
+      $compile('<div md-deferred-theme="unregistered"></div>')($rootScope);


You actually would want this to warn because the theme isn't registered. This approach loses that information.

ErinCoughlan · 2016-08-26T20:58:19Z

@EladBezalel I have another idea. Instead of using a string in md-theme, can you use an object, at least in the deferred class? That way we know if it will be created since the object needs to come from somewhere.

devversion · 2016-08-31T12:19:57Z

src/core/services/theming/theming.js

-          }
+            // Iterating backwards to support unregistering during iteration
+            // http://stackoverflow.com/a/9882349/890293
+            for (var i = registeredCallbacks.length - 1; i >= 0; i -= 1) {


Doesn't the following work?

registeredCallbacks.reverse().forEach(function(callback) {

Note: forEach keeps the right order.

Maybe 😅 i'll check

ThomasBurleson · 2016-08-31T14:36:41Z

src/core/services/theming/theming.js

        // 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', {


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

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

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.

topherfangio · 2016-08-31T15:44:21Z

Looks pretty interesting. Any usage docs so that we can play with it and test it out? 😃

EladBezalel · 2016-08-31T16:02:30Z

Actually I was testing on button demo, had a timeout to resolve a new defined theme after 2 seconds

topherfangio · 2016-08-31T16:21:09Z

@EladBezalel Ok. Can you share that code at least so that we can do some testing on our local machines?

Also, this will need docs before we merge it, right?

EladBezalel · 2016-08-31T16:28:06Z

There are no docs for the theming service at all.. I wanted to add it but it requires even more work on that..
I can fix the comments and work on another PR for documentation to mdTheming service

EladBezalel · 2016-08-31T16:43:09Z

Run gulp watch site --dev and go to
http://codepen.io/EladBezalel/pen/YGKBYE
You can also remove md-theme-watch to see how it's not being changed

ErinCoughlan · 2016-08-31T17:02:38Z

src/core/services/theming/theming.js

-      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'));


nit: line too long here :)

ErinCoughlan · 2016-08-31T17:12:49Z

src/core/services/theming/theming.spec.js

+
+  it('should accept $q promise as a theme', function() {
+    inject(function($compile, $rootScope, $q, $mdTheming) {
+      $rootScope.promise = $q.resolve('red');


So I thought there was a method on $mdTheming that returns a promise (which eventually is the resolve). To make this test more robust and closer to real use, can you use that method instead of just resolve with a string.

ErinCoughlan · 2016-08-31T17:40:02Z

src/core/services/theming/theming.spec.js

@@ -453,6 +453,7 @@ describe('$mdThemeProvider with custom styles', function() {
      // Verify that $MD_THEME_CSS is still set to '/**/' in the test environment.
      // Check angular-material-mocks.js for $MD_THEME_CSS latest value if this test starts to fail.
      expect($MD_THEME_CSS).toBe('/**/');
+      expect($MD_THEME_CSS).toBe('/**/');


duplicate line?

ErinCoughlan · 2016-08-31T17:42:08Z

LGTM

devversion · 2016-08-31T17:42:14Z

src/core/services/theming/theming.js

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


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

It's still explaining why are we iterating backwards

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

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.

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.

- Added `defineTheme` functionality to `$mdTheming` service - Added support for promise and a function that returns a promise on `md-theme` directive to support async build of themes fixes #2965

This reverts commit 0d2386c.

EladBezalel added the needs: review This PR is waiting on review from the team label Aug 26, 2016

EladBezalel assigned ThomasBurleson and ErinCoughlan Aug 26, 2016

EladBezalel force-pushed the feat/themes-on-the-fly branch from e0ecde8 to 9310a84 Compare August 26, 2016 11:19

EladBezalel added needs: work and removed needs: review This PR is waiting on review from the team labels Aug 26, 2016

ErinCoughlan reviewed Aug 26, 2016
View reviewed changes

EladBezalel force-pushed the feat/themes-on-the-fly branch from 9310a84 to 3910466 Compare August 28, 2016 12:32

EladBezalel added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Aug 28, 2016

EladBezalel force-pushed the feat/themes-on-the-fly branch 3 times, most recently from 2681002 to c72246e Compare August 31, 2016 12:10

EladBezalel added needs: review This PR is waiting on review from the team and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: work labels Aug 31, 2016

devversion reviewed Aug 31, 2016
View reviewed changes

EladBezalel force-pushed the feat/themes-on-the-fly branch from c72246e to ac16f25 Compare August 31, 2016 12:31

ThomasBurleson reviewed Aug 31, 2016
View reviewed changes

ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Aug 31, 2016

ThomasBurleson added this to the 1.1.1 milestone Aug 31, 2016

ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Aug 31, 2016

EladBezalel force-pushed the feat/themes-on-the-fly branch from ac16f25 to f6e667e Compare August 31, 2016 16:45

ErinCoughlan reviewed Aug 31, 2016
View reviewed changes

ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Aug 31, 2016

ErinCoughlan reviewed Aug 31, 2016
View reviewed changes

EladBezalel force-pushed the feat/themes-on-the-fly branch 3 times, most recently from 54328ae to 72ff924 Compare August 31, 2016 17:24

ErinCoughlan reviewed Aug 31, 2016
View reviewed changes

devversion reviewed Aug 31, 2016
View reviewed changes

EladBezalel force-pushed the feat/themes-on-the-fly branch from 72ff924 to 6837c1b Compare August 31, 2016 17:47

feat(themes): register theme on the fly

433a305

- Added `defineTheme` functionality to `$mdTheming` service - Added support for promise and a function that returns a promise on `md-theme` directive to support async build of themes fixes #2965

EladBezalel force-pushed the feat/themes-on-the-fly branch from 6837c1b to 433a305 Compare August 31, 2016 18:05

hansl merged commit 0d2386c into master Aug 31, 2016

EladBezalel deleted the feat/themes-on-the-fly branch August 31, 2016 22:22

hansl added a commit to hansl/material that referenced this pull request Aug 31, 2016

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

6f75c55

This reverts commit 0d2386c.

ThomasBurleson pushed a commit that referenced this pull request Aug 31, 2016

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

3854c4a

This reverts commit 0d2386c.

EladBezalel restored the feat/themes-on-the-fly branch August 31, 2016 23:32

EladBezalel mentioned this pull request Sep 1, 2016

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

Merged

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

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

Uh oh!

Conversation

EladBezalel commented Aug 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EladBezalel commented Aug 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan commented Aug 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hansl Aug 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topherfangio commented Aug 31, 2016

Uh oh!

EladBezalel commented Aug 31, 2016

Uh oh!

topherfangio commented Aug 31, 2016

Uh oh!

EladBezalel commented Aug 31, 2016

Uh oh!

EladBezalel commented Aug 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan commented Aug 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion Aug 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion Aug 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EladBezalel commented Aug 26, 2016 •

edited

Loading

hansl Aug 31, 2016 •

edited

Loading

EladBezalel commented Aug 31, 2016 •

edited

Loading

devversion Aug 31, 2016 •

edited

Loading

devversion Aug 31, 2016 •

edited

Loading