The Wayback Machine - https://web.archive.org/web/20201130105001/https://github.com/angular/material/issues/11229
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dialog: theme not inheriting properly when changing themes #11229

Open
Arns opened this issue Apr 11, 2018 · 6 comments
Open

dialog: theme not inheriting properly when changing themes #11229

Arns opened this issue Apr 11, 2018 · 6 comments

Comments

@Arns
Copy link

@Arns Arns commented Apr 11, 2018

Bug, feature request, or proposal:

Bug

What is the expected behavior?

The dialog will take on the theme that is set on its parent element (or somewhere in the ascendants).

What is the current behavior?

If changing the theme, the dialog still uses the previous theme.

CodePen and steps to reproduce the issue:

CodePen Demo which shows your issue: https://codepen.io/anon/pen/WzmmKx
Detailed Reproduction Steps:
  1. set a theme
  2. open dialog WITHOUT targetEvent set with an md-primary button and view theme
  3. switch theme using theme provider/service
  4. open dialog and see that theme did not update

What is the use-case or motivation for changing an existing behavior?

It essentially renders theme changing useless since the interim elements don't respect the change.

Which versions of AngularJS, Material, OS, and browsers are affected?

AngularJS Material >= 1.1.2
AngularJS >= 1.6.0

Is there anything else we should know? Stack Traces, Screenshots, etc.

Likely something in the following changes that broke it: https://github.com/angular/material/pull/9762/files#diff-58cbee594b2fabf2c78b6d283750e535R806

@Arns
Copy link
Author

@Arns Arns commented Apr 11, 2018

Originally I did not realize this, but it appears a work around is setting the targetEvent when opening a dialog via the $mdDialog service. Not sure why/how this is related, and it is not documented anywhere that I've found.

@Splaktar
Copy link
Member

@Splaktar Splaktar commented Apr 17, 2018

I updated the CodePen to 1.1.8 and I still see the issue. Thank you for reporting this and for creating the CodePen.

@Splaktar Splaktar self-assigned this Apr 17, 2018
@Splaktar Splaktar added this to the 1.1.10 milestone Apr 17, 2018
@Splaktar Splaktar changed the title md-dialog theme not working/inheriting properly when changing themes dialog: theme not inheriting properly when changing themes Apr 17, 2018
@Splaktar Splaktar modified the milestones: 1.1.10, 1.1.11 Jun 19, 2018
@Splaktar Splaktar modified the milestones: 1.1.11, 1.1.12 Sep 10, 2018
@Splaktar Splaktar modified the milestones: 1.1.12, 1.1.13 Jan 3, 2019
@Splaktar Splaktar modified the milestones: 1.1.13, 1.1.14 Feb 10, 2019
@Splaktar Splaktar added the g3: sync label Feb 15, 2019
@Splaktar Splaktar modified the milestones: 1.1.14, g3: sync Feb 15, 2019
@jelbourn jelbourn added P4: minor and removed P2: required labels Feb 22, 2019
@Splaktar Splaktar modified the milestones: g3: sync, 1.1.23 Apr 30, 2020
@Splaktar Splaktar modified the milestones: 1.1.23, 1.2.1 Jun 8, 2020
@tbitai
Copy link

@tbitai tbitai commented Jul 19, 2020

Originally I did not realize this, but it appears a work around is setting the targetEvent when opening a dialog via the $mdDialog service. Not sure why/how this is related, and it is not documented anywhere that I've found.

Another workaround – just set the theme explicitly, like so:

let alert = $mdDialog.alert({
  title: 'Themed',
  textContent: 'This dialog is correctly themed!',
  ok: 'Close',
  theme: $scope.theme
});
$mdDialog.show(alert);

$mdDialogPreset#theme is documented under the preset creators, e.g. $mdDialog.alert().

This workaround also works when you can't set targetEvent (this was how I found it out), and it's more stable as it doesn't rely on the not well understood connection with targetEvent, which might disappear in next versions of AngularJS Material.

@Splaktar
Copy link
Member

@Splaktar Splaktar commented Jul 21, 2020

$mdDialog's Theme Inheritance Demo uses the targetEvent API, which is required for theme inheritance.

However, while this API is documented, it isn't documented that this API is required for theme inheritance. A note about this needs to be added to the $mdDialog service docs. I don't think that it makes sense directly on the targetEvent API. We should probably add a dialog theming example to these docs and a note about specifying targetEvent being required.

We should also have a brief example of theming dialogs using the theme API w/o inheritance (as mentioned in the previous comment).

@Splaktar Splaktar added type: docs and removed type: bug labels Jul 21, 2020
@tbitai
Copy link

@tbitai tbitai commented Jul 23, 2020

So do I understand correctly that theme inheritance works like theme is inherited from the element which corresponds to the DOMClickEvent passed as targetEvent?

Anyway, information about dialogs' theme inheritance mechanism would be nice in the theming docs as well, as for example for me it was surprising that I set an md-theme on my body and it didn't propagate to the dialogs, despite that they're children of body.

(The logic of my app is so that it doesn't make sense for my dialogs to have a targetEvent. If I understand correctly, in that case theme inheritance is not defined according to AngularJS Material's system, so I have to explicitly set the theme, which is fine, just it would be nice if this was documented.)

As I think about it, theme inheritance via targetEvent is actually reasonable, as there might be multiple themes present among the elements of the page, and then $mdDialog wouldn't know which theme the dialog should inherit. Although perhaps it could be set to fall back to parent's theme in case targetEvent is not set, and then the explicit setting I mentioned above wouldn't be necessary.

@Splaktar
Copy link
Member

@Splaktar Splaktar commented Jul 23, 2020

So do I understand correctly that theme inheritance works like theme is inherited from the element which corresponds to the DOMClickEvent passed as targetEvent?

Yes, see

function detectTheming(options) {
// Once the user specifies a targetEvent, we will automatically try to find the correct
// nested theme.
var targetEl;
if (options.targetEvent && options.targetEvent.target) {
targetEl = angular.element(options.targetEvent.target);
}
var themeCtrl = targetEl && targetEl.controller('mdTheme');
options.hasTheme = (!!themeCtrl);
if (!options.hasTheme) {
return;
}

Agreed that it should be mentioned with a note/callout in the theming guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.