feat(dialog themes): extended theme inheritance of dialogs by EladBezalel · Pull Request #9762 · angular/material

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

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

+        };
+
+        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

+
+        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

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


angular.isFunction(value); perhaps

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

-        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

  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

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

    $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


		setParsedTheme(getTheme());

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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