Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(mdPanel): Panel and tooltip theming #10031

Merged
merged 1 commit into from
Nov 29, 2016
Merged

fix(mdPanel): Panel and tooltip theming #10031

merged 1 commit into from
Nov 29, 2016

Conversation

bradrich
Copy link
Contributor

  • Initialized theming through $mdTheming on the panel element and container after the panel is created.
  • Fixed the panel and tooltip theme Sass files to remove unthemed styles and complicated syntax.
  • Removed an unnecessary $mdTheming call from the tooltip directive.

Fixes #10030

@bradrich bradrich added this to the 1.1.1 milestone Nov 17, 2016
@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Nov 17, 2016
@bradrich bradrich modified the milestones: 1.1.2, 1.1.1 Nov 17, 2016
@ErinCoughlan
Copy link
Contributor

This appears to fix the tooltip, but not the panel backdrop. The theme is still not applied to the backdrop.

@bradrich
Copy link
Contributor Author

It seemed to work in my tests. How are you isolating it to get these results?

@bradrich
Copy link
Contributor Author

Were you able to see where the $mdTheming calls were made on the panelContainer and the panelEl? The panelContainer is the backdrop of the panel API?

@ErinCoughlan
Copy link
Contributor

I tested this by running the presubmit and patching it into our code where I could see the tooltip and a standalone panel.

So you can also see the standalone panel, if you temporarily add hasBackdrop to the tooltip panel config, no backdrop appears.

@ErinCoughlan
Copy link
Contributor

For more information, mdTheming is called for the tooltip panel and the tooltip outer wrapper, but not for the backdrop panel or the backdrop outer wrapper.

@ErinCoughlan
Copy link
Contributor

Ahha! Got it!

For the backdrop, there is no positionConfig, so the addStyles method resolves without hitting the $mdTheming call.

If you add the two $mdTheming lines at the beginning of hideAndResolve in addition to the current location, the backdrop appears the right color.

@bradrich
Copy link
Contributor Author

Very cool. I am updating this now.

@bradrich
Copy link
Contributor Author

Would it be better to put it here: https://github.com/angular/material/blob/master/src/components/panel/panel.js#L1662? The hideAndResolve function could be called more than once. However, I don't think that the $mdTheming call happening more than once is a bad thing...

@bradrich bradrich force-pushed the fix/panel-tooltip-theme branch from 40a376a to 5e3cead Compare November 17, 2016 21:22
@bradrich
Copy link
Contributor Author

@ErinCoughlan Give this a check and see what you think.

ErinCoughlan
ErinCoughlan approved these changes Nov 17, 2016
View reviewed changes
Copy link
Contributor

@ErinCoughlan ErinCoughlan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/components/panel/panel.js
resolve(self);
});
});
};


/**
* Sets the `$mdTheming` classes on the `panelContainer` and `panelEl`.
* @method setTheming
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line isn't needed, @method is only used for the ngdoc, but this isn't public.

@bradrich
fix(mdPanel): Panel and tooltip theming
d30c26e 
 - Initialized theming through `$mdTheming` on the panel element
   and container after the panel is created.
 - Fixed the panel and tooltip theme Sass files to remove unthemed
   styles and complicated syntax.
 - Removed an unnecessary `$mdTheming` call from the tooltip
   directive.

Fixes angular#10030
@bradrich bradrich force-pushed the fix/panel-tooltip-theme branch from 5e3cead to d30c26e Compare November 18, 2016 00:29
@ThomasBurleson ThomasBurleson added P0: critical Critical issues that must be addressed immediately. P0 - Presubmit First has: Pull Request A PR has been created to address this issue labels Nov 21, 2016
@ThomasBurleson
Copy link
Contributor

@jelbourn - important fix.

@jelbourn jelbourn merged commit b8357dc into angular:master Nov 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ErinCoughlan ErinCoughlan ErinCoughlan approved these changes

Assignees

@ErinCoughlan ErinCoughlan

@bradrich bradrich

@clshortfuse clshortfuse

Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ has: Pull Request A PR has been created to address this issue P0: critical Critical issues that must be addressed immediately.
Projects
None yet
Milestone
1.1.2
Development

Successfully merging this pull request may close these issues.
6 participants
@bradrich @ErinCoughlan @ThomasBurleson @jelbourn @googlebot @clshortfuse