feat(dialog themes): extended theme inheritance of dialogs #9762

EladBezalel · 2016-10-05T19:16:26Z

Dialogs now can also inherit promise/function defined themes and listen to the targetElement theme changes
Removed wrong tests, we now assume that if an interpolation is being used in md-theme it was meant to be watched,
unless one-way binding (::) was applied.

Related to #9475

Note: rebased on #9475 until it would be merged in.

cc @ErinCoughlan, @ThomasBurleson, @topherfangio

ErinCoughlan · 2016-10-06T17:18:18Z

src/components/dialog/demoThemeInheritance/style.css

@@ -0,0 +1,3 @@
+.container {
+  text-align: center;
+}


nit: No newline at end of file

Do you mind explaining me the ideology behind this? 😓

Github tells me when there isn't a new line with a nice red icon, so I was trying to make it happy.

ErinCoughlan · 2016-10-06T18:25:36Z

src/components/dialog/dialog.spec.js

+        };
+
+        button[0].click();
+        $rootScope.$apply();


You shouldn't need an $apply after the click, since clicking triggers angular event update.

Ditto in other places too.

ErinCoughlan · 2016-10-06T18:27:46Z

src/core/services/theming/theming.js

+
+        setParsedTheme(getTheme());
+
+        var unwatch = scope.$watch(getTheme, function (theme) {


nit: no space after function (Everywhere... it's inconsistent with the rest of the files)

ErinCoughlan · 2016-10-06T18:28:23Z

src/core/services/theming/theming.js

+            return ctrl.$setTheme(theme);
+          }
+
+          $q.when( (typeof theme === 'function') ?  theme() : theme )


angular.isFunction(value); perhaps

ErinCoughlan · 2016-10-06T18:29:04Z

src/core/services/theming/theming.js

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


Indentation on this line should be 4, not 2. Also consider splitting this onto multiple lines.

ErinCoughlan · 2016-10-06T18:30:41Z

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

@@ -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) {


Maybe move the inject part on a newline to help with readability and line length.

This is also inconsistent within this file.

ThomasBurleson · 2016-10-09T11:57:27Z

Let's avoid nitpicks for spacing, indentation, etc.
Readability and consistency are important.

ThomasBurleson · 2016-10-09T11:58:35Z

@EladBezalel - please squash commits and rebase.

@EladBezalel - comments also need breaking change information.

EladBezalel · 2016-10-09T13:38:05Z

@ErinCoughlan please rereview

ErinCoughlan · 2016-10-10T16:20:19Z

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

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


This will be slightly more obvious that it is a promise if you wrap it in one. Otherwise, I have to look at the definition of defineTheme to verify that this test is testing what it says to.

Ditto for the other "returns a promise" tests.

ErinCoughlan · 2016-10-10T16:22:43Z

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

    $rootScope.themey = 'red';
    var el = $compile('<div md-theme="{{themey}}"><span md-themable></span></div>')($rootScope);
    $rootScope.$apply();

    expect(el.children().hasClass('md-red-theme')).toBe(true);
    $rootScope.$apply('themey = "blue"');
-    expect(el.children().hasClass('md-blue-theme')).toBe(false);
-    expect(el.children().hasClass('md-red-theme')).toBe(true);
+    expect(el.children().hasClass('md-blue-theme')).toBe(true);


I'm wondering if you want to make these classes more generic, perhaps using toContain('blue').

This test relies on the knowledge that md-theme attribute adds a class in this specific format. If you ever change the format, the test will fail, when I'm not sure it should.

ErinCoughlan · 2016-10-10T16:23:48Z

@ThomasBurleson Apologies for the nitpicks. I mainly only commented when two functions in a row were formatted differently. I will try not to comment on them any more.

@EladBezalel Done the review. In general, looks good to me with a few test improvements. If you rebase, I can run the presubmit and see if there are any unexpected changes I didn't notice.

- Dialogs now can also inherit promise/function defined themes and listen to the targetElement theme changes - Removed wrong tests, we now assume that if an interpolation is being used in `md-theme` it was meant to be watched, unless one-way binding (::) was applied. Related to #9475

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

EladBezalel force-pushed the feat/dialog-theme-inheritance branch from 126a5d2 to 9f3b8fa Compare October 5, 2016 19:56

ErinCoughlan reviewed Oct 6, 2016

View reviewed changes

EladBezalel force-pushed the feat/dialog-theme-inheritance branch from 9f3b8fa to 23a08bb Compare October 7, 2016 19:35

EladBezalel assigned ThomasBurleson Oct 7, 2016

ThomasBurleson added needs: presubmit and removed needs: review This PR is waiting on review from the team labels Oct 9, 2016

ThomasBurleson added this to the 1.1.2 milestone Oct 9, 2016

ThomasBurleson removed the needs: presubmit label Oct 9, 2016

ThomasBurleson added needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Oct 9, 2016

ErinCoughlan reviewed Oct 10, 2016

View reviewed changes

EladBezalel force-pushed the feat/dialog-theme-inheritance branch from 23a08bb to fd25c3c Compare October 17, 2016 17:05

EladBezalel added needs: presubmit and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved labels Oct 17, 2016

EladBezalel force-pushed the feat/dialog-theme-inheritance branch from fd25c3c to a65e080 Compare October 25, 2016 18:42

jelbourn merged commit b7ae33e into master Oct 27, 2016

EladBezalel deleted the feat/dialog-theme-inheritance branch October 28, 2016 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(dialog themes): extended theme inheritance of dialogs #9762

feat(dialog themes): extended theme inheritance of dialogs #9762

Uh oh!

EladBezalel commented Oct 5, 2016

Uh oh!

ErinCoughlan Oct 6, 2016

Uh oh!

EladBezalel Oct 6, 2016

Uh oh!

ErinCoughlan Oct 6, 2016

Uh oh!

ErinCoughlan Oct 6, 2016 •

edited

Loading

Uh oh!

ErinCoughlan Oct 6, 2016 •

edited

Loading

Uh oh!

ErinCoughlan Oct 6, 2016

Uh oh!

ErinCoughlan Oct 6, 2016

Uh oh!

ErinCoughlan Oct 6, 2016 •

edited

Loading

Uh oh!

ThomasBurleson commented Oct 9, 2016

Uh oh!

ThomasBurleson commented Oct 9, 2016 •

edited

Loading

Uh oh!

EladBezalel commented Oct 9, 2016

Uh oh!

ErinCoughlan Oct 10, 2016 •

edited

Loading

Uh oh!

ErinCoughlan Oct 10, 2016

Uh oh!

ErinCoughlan commented Oct 10, 2016

Uh oh!

Uh oh!


		setParsedTheme(getTheme());

		var unwatch = scope.$watch(getTheme, function (theme) {

feat(dialog themes): extended theme inheritance of dialogs #9762

feat(dialog themes): extended theme inheritance of dialogs #9762

Uh oh!

Conversation

EladBezalel commented Oct 5, 2016

Uh oh!

ErinCoughlan Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

EladBezalel Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan Oct 6, 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!

ErinCoughlan Oct 6, 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!

ErinCoughlan Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan Oct 6, 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!

ThomasBurleson commented Oct 9, 2016

Uh oh!

ThomasBurleson commented Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EladBezalel commented Oct 9, 2016

Uh oh!

ErinCoughlan Oct 10, 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!

ErinCoughlan Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

ErinCoughlan commented Oct 10, 2016

Uh oh!

Uh oh!

ErinCoughlan Oct 6, 2016 •

edited

Loading

ErinCoughlan Oct 6, 2016 •

edited

Loading

ErinCoughlan Oct 6, 2016 •

edited

Loading

ThomasBurleson commented Oct 9, 2016 •

edited

Loading

ErinCoughlan Oct 10, 2016 •

edited

Loading