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

fix(menu): md-menu panel theme supports dark mode#11230

Merged
andrewseguin merged 1 commit into
angular:masterfrom
rudzikdawid:fixMenuPopupTheme
Apr 23, 2018
Merged

fix(menu): md-menu panel theme supports dark mode#11230
andrewseguin merged 1 commit into
angular:masterfrom
rudzikdawid:fixMenuPopupTheme

Conversation

@rudzikdawid
Copy link
Copy Markdown
Contributor

@rudzikdawid rudzikdawid commented Apr 11, 2018

Closes #11199

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?

The md-menu panel is always using light mode hues even when the theme is dark mode.

Issue Number:
Fixes #11199.

What is the new behavior?

The md-menu panel use dark mode hues when the theme is set to dark mode.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Dark before:

Dark after:

Light before:

Light after:

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Apr 11, 2018
@rudzikdawid rudzikdawid mentioned this pull request Apr 12, 2018
Closed
@Splaktar Splaktar self-requested a review April 16, 2018 22:14
@Splaktar Splaktar added type: bug P4: minor Minor issues. May not be fixed without community contributions. ui: theme labels Apr 16, 2018
@Splaktar Splaktar added this to the 1.1.9 milestone Apr 16, 2018
@Splaktar Splaktar mentioned this pull request Apr 16, 2018
Closed
@Splaktar
Copy link
Copy Markdown
Contributor

Splaktar commented Apr 16, 2018

md-menu-bar needs some help too. I opened a separate issue for that: #11238.

Splaktar
Splaktar suggested changes Apr 16, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Looks good. I just want to remove the one redundant opacity setting. Thanks!

Comment thread src/components/menu/menu-theme.scss

md-menu-divider {
background-color: '{{background-A200-0.11}}';
background-color: '{{foreground-4}}';
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar Apr 16, 2018

Choose a reason for hiding this comment

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

background-hue-3-0.79 seems to get me the same color as the current light mode rgb(227, 227, 227). But doesn't work for dark mode (as it's too dark). This currently gets me rgb(224, 224, 224) which is slightly darker, but is probably close enough.

It doesn't appear that we can use opacity on foreground hues like we can with background hues. So I think that this is the best we can do with the current theming support.

This also lines up with the comments in theming.js: {{foreground-4}} - used for dividers.

Copy link
Copy Markdown
Contributor Author

@rudzikdawid rudzikdawid Apr 17, 2018

Choose a reason for hiding this comment

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

yep that is why i used {{foreground-4}} looks good in both modes dark/light and also is recommended directly in theming.js

Comment thread src/components/menu/menu-theme.scss Outdated

md-menu-item {
color: '{{background-A200-0.87}}';
color: '{{foreground-1-0.87}}';
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar Apr 16, 2018

Choose a reason for hiding this comment

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

The -0.87 is redundant here and can be removed. For light mode, foreground-1 is rgba(0,0,0,0.87) and for dark mode it is rgba(255,255,255,1.0).

Copy link
Copy Markdown
Contributor Author

@rudzikdawid rudzikdawid Apr 17, 2018

Choose a reason for hiding this comment

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

fixed

@Splaktar Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Apr 16, 2018
Splaktar
Splaktar approved these changes Apr 17, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Apr 17, 2018
@andrewseguin andrewseguin merged commit ef14194 into angular:master Apr 23, 2018
@mmalerba mmalerba removed their assignment Apr 23, 2018
Splaktar pushed a commit that referenced this pull request Jul 31, 2018
@rudzikdawid @Splaktar
fix(menu): md-menu panel theme supports dark mode (#11230)
f4607ae 
Closes #11199
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@Splaktar Splaktar Splaktar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@andrewseguin andrewseguin

Labels

cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P4: minor Minor issues. May not be fixed without community contributions. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review type: bug ui: theme

Projects

None yet

Milestone

1.1.9

Development

Successfully merging this pull request may close these issues.

menu: dropdown doesn't inherit dark theme

5 participants

@rudzikdawid @Splaktar @googlebot @mmalerba @andrewseguin