fix(dialog) md-colors breaking inside of dialogs #11078

codymikol · 2018-01-18T19:26:17Z

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Currently when you display an mdDialog, the directive tries to see if an md-theme controller exists from where the mdDialog was propogated. The directive then adds an md-theme attrubute to a newly created div inside of md-dialog. The directive does this regardless of whether there is an inherited theme or not.

This is an issue because the md-theme gets set to an empty string and when attempting to use md-colors, it recognizes the empty string as a set theme and incorrectly parses

<div md-colors="{background:'primary'}">

as -primary instead of just primary.

Issue Number: #10276

What is the new behavior?

The md-theme attribute will only be set if a theme actually exists on the element from which
md-dialog is propogated. This allows default themes to work as expected inside of the dialogs.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

No breaking changes.

Other information

googlebot · 2018-01-18T19:26:20Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

codymikol · 2018-01-18T19:35:10Z

I hopefully fixed my CLA?!

codymikol · 2018-01-18T19:47:54Z

I really did it this time!

googlebot · 2018-01-18T19:47:57Z

CLAs look good, thanks!

codymikol · 2018-02-28T21:29:26Z

Here is a link to the issue: #10276

Here is a demo of the issue: https://codepen.io/lost_semicolon/pen/appyEG

(Taken from the Issue, thanks @mohammadrafigh)

Splaktar

Thank you very much for your contribution! This looks very good. I just had a few nits, the biggest things that need to be addressed are squashing the commits and more closely following the commit message guidelines.

Please use "Fixes #10276" in the footer and the first line should be "fix(dialog): md-colors breaks inside of dialogs".

Splaktar · 2018-03-01T04:43:18Z

src/components/dialog/dialog.spec.js

@@ -1774,6 +1774,57 @@ describe('$mdDialog', function() {
    });

    describe('theming', function () {
+
+        it('should inherit md-theme if the child has and md-theme to inherit', inject(function ($mdDialog, $mdTheming, $rootScope, $compile) {


has and md-theme => has a md-theme

👍 Looks good now

Splaktar · 2018-03-01T04:51:49Z

src/components/dialog/dialog.spec.js

+
+        }));
+
+        it('should not set md-theme if the child does not have md-theme to inherit', inject(function ($mdDialog, $mdTheming, $rootScope, $compile) {


Please wrap new lines added in this PR at 120 or less.

👍 Looks good now

Splaktar · 2018-03-01T04:52:33Z

src/components/dialog/dialog.spec.js

+            var container = angular.element(parent[0].querySelector('.md-dialog-container'));
+
+            expect(container.attr('md-theme')).toBeUndefined();
+


Please remove this blank line.

👍 Looks good now

Splaktar · 2018-03-01T04:52:42Z

src/components/dialog/dialog.spec.js

+            var container = angular.element(parent[0].querySelector('.md-dialog-container'));
+
+            expect(container.attr('md-theme')).toEqual('coolTheme');
+


Please remove this blank line.

👍 Looks good now

Only append an md-theme attribute if the md-dialog is propagated from an element that contains an md-theme. Fixes angular#10276

codymikol · 2018-03-01T18:49:33Z

@Splaktar I believe have addressed all of the changes you requested, let me know if anything else needs tweaking. 🐔

Splaktar

LGTM

Splaktar · 2018-03-15T01:00:40Z

My first request for this PR to be presubmitted on 3/3 got lost. It will be run through presubmit tomorrow. Sorry for the delay!

Only append an md-theme attribute if the md-dialog is propagated from an element that contains an md-theme. Fixes angular#10276

Only append an md-theme attribute if the md-dialog is propagated from an element that contains an md-theme. Fixes #10276

googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Jan 18, 2018

codymikol mentioned this pull request Jan 18, 2018

md-colors is broken with default theme in dialog #10276

Closed

googlebot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Jan 18, 2018

codymikol changed the title ~~Fix md-colors breaking inside md-dialog~~ (fix)md-dialog: md-colors breaking inside of dialogs Jan 18, 2018

codymikol changed the title ~~(fix)md-dialog: md-colors breaking inside of dialogs~~ fix(md-dialog) md-colors breaking inside of dialogs Jan 18, 2018

codymikol changed the title ~~fix(md-dialog) md-colors breaking inside of dialogs~~ fix(dialog) md-colors breaking inside of dialogs Jan 18, 2018

Splaktar self-requested a review March 1, 2018 04:35

Splaktar self-assigned this Mar 1, 2018

Splaktar added this to the 1.1.8 milestone Mar 1, 2018

Splaktar added P2: required Issues that must be fixed. severity: regression This issue is related to a regression ui: theme type: bug needs: squash commits labels Mar 1, 2018

Splaktar suggested changes Mar 1, 2018

View reviewed changes

Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Mar 1, 2018

fix(dialog): md-colors breaks inside of dialogs

49593c8

Only append an md-theme attribute if the md-dialog is propagated from an element that contains an md-theme. Fixes angular#10276

codymikol force-pushed the md-dialog-child-propogation-bugfix branch from c32eaf1 to 49593c8 Compare March 1, 2018 18:28

Splaktar 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 needs: squash commits labels Mar 3, 2018

Splaktar approved these changes Mar 3, 2018

View reviewed changes

Splaktar added the pr: lgtm This PR has been approved by the reviewer label Mar 3, 2018

codymikol mentioned this pull request Mar 14, 2018

Modals do not appear to respect theming #525

Closed

mmalerba merged commit e3c55f9 into angular:master Mar 16, 2018

Splaktar mentioned this pull request Mar 16, 2018

docs(changelog): add md-selected-text not accepting HTML anymore as a breaking change in 1.1.5 #10912

Closed

Splaktar pushed a commit that referenced this pull request Jul 31, 2018

fix(dialog) md-colors breaking inside of dialogs (#11078)

e2d0623

Only append an md-theme attribute if the md-dialog is propagated from an element that contains an md-theme. Fixes #10276


		}));

		it('should not set md-theme if the child does not have md-theme to inherit', inject(function ($mdDialog, $mdTheming, $rootScope, $compile) {

		var container = angular.element(parent[0].querySelector('.md-dialog-container'));

		expect(container.attr('md-theme')).toBeUndefined();

		var container = angular.element(parent[0].querySelector('.md-dialog-container'));

		expect(container.attr('md-theme')).toEqual('coolTheme');

fix(dialog) md-colors breaking inside of dialogs #11078

fix(dialog) md-colors breaking inside of dialogs #11078

Uh oh!

Conversation

codymikol commented Jan 18, 2018 • edited by Splaktar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

googlebot commented Jan 18, 2018

Uh oh!

codymikol commented Jan 18, 2018

Uh oh!

codymikol commented Jan 18, 2018

Uh oh!

googlebot commented Jan 18, 2018

Uh oh!

codymikol commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Splaktar left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

codymikol commented Mar 1, 2018

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Splaktar commented Mar 15, 2018

Uh oh!

Uh oh!

codymikol commented Jan 18, 2018 •

edited by Splaktar

Loading

codymikol commented Feb 28, 2018 •

edited

Loading