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

fix(select): options panel theme supports dark mode #11198

Merged
merged 1 commit into from
Apr 23, 2018

Conversation

Splaktar
Copy link
Contributor

@Splaktar Splaktar commented Mar 28, 2018

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 select's option panel is always using light mode hues even when the theme is dark mode.

Issue Number:
Fixes #3379.

What is the new behavior?

The select's option 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

I'm going to try and presubmit this for 1.1.9, but I understand that this might trigger some screenshot failures in Google apps. If that happens, we'll need to evaluate pushing this to 1.2.0.

Dark Before:
screen shot 2018-03-28 at 5 59 25 pm

Dark After with Item 1 selected:
screen shot 2018-04-07 at 4 05 18 am

Light Before:
screen shot 2018-04-07 at 4 04 37 am

Light After:
screen shot 2018-04-07 at 4 07 55 am

@Splaktar Splaktar added ui: theme pr: merge ready This PR is ready for a caretaker to review P4: minor Minor issues. May not be fixed without community contributions. labels Mar 28, 2018
@Splaktar Splaktar added this to the 1.1.9 milestone Mar 28, 2018
@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Mar 28, 2018
@Splaktar Splaktar added the needs: team discussion This issue requires a decision from the team before moving forward. label Mar 30, 2018
@rudzikdawid
Copy link
Contributor

rudzikdawid commented Apr 5, 2018

same problem as here: #11203

before:

after:

Please ensure that the light mode colors don't change.

also darker hover background color in dark theme looks weird:

&:hover {background: '{{background-hue-2}}'}

material2 has lighter hover background color:

Also official material stylguide seems to recomend lighter hover selection:

https://material.io/guidelines/components/buttons.html#buttons-toggle-buttons

@rudzikdawid
Copy link
Contributor

rudzikdawid commented Apr 5, 2018

i think that for hover: background: '{{background-500-0.17}}'; should be much better instead '{{background-hue-2}}'

in light mode its almost look the same as master build.
but in dark mode we have lighter hover background color similar to material2 implementation and it's much closer to material official styleguide

@Splaktar Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed pr: merge ready This PR is ready for a caretaker to review needs: team discussion This issue requires a decision from the team before moving forward. labels Apr 7, 2018
@Splaktar Splaktar force-pushed the fixSelectPopupTheme branch from 2333deb to 9c19496 Compare April 7, 2018 08:03
@Splaktar 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 labels Apr 7, 2018
@Splaktar
Copy link
Contributor Author

Splaktar commented Apr 7, 2018

@rudzikdawid thank you for looking into that! I did some further testing and settled on background-500-0.20. I've updated the OP with the new screenshots for dark and light modes.

@rudzikdawid
Copy link
Contributor

With background-500-0.20 there is is clear difference between this version and master build (last release). background-500-0.17 is almost the same background color as master build. And it's hard to see any difference in light mode

@Splaktar
Copy link
Contributor Author

Splaktar commented Apr 7, 2018

Strange, it may be a monitor or OS difference then. I see the opposite where 0.17 is a clear difference (lighter) from what's on master.

@Splaktar
Copy link
Contributor Author

Splaktar commented Apr 7, 2018

Using ColorZilla give me master as 235, 235, 235 and 0.20 as 234, 234, 234. So it actually needs to be even slightly more opaque than 0.20 to be the same.

@rudzikdawid
Copy link
Contributor

rudzikdawid commented Apr 7, 2018

weird, on my fedora 27, chrome 65, firefox 59 master build: https://material.angularjs.org/latest/demo/select
i have: rgb(238,238,238) instead rgb(235, 235, 235)

devtools on master shows:

md-select-menu md-content md-option:not([disabled]):hover {
    background: rgb(238,238,238);
}

hover changes with 0.17:

li:hover {
    background: rgba(158,158,158,0.17);
}

i use converter rgba to rgb
http://marcodiiga.github.io/rgba-to-rgb-conversion

result of convert:
0.17 background: rgba(158,158,158,0.17); === background: rgb(239,239,239);
0.20 background: rgba(158,158,158,0.20); === background: rgb(236,236,236);

master build: background: rgb(238,238,238);

results of difference between master:
0.17 differ by 1 point (lighter)
0.20 differ by 2 point (darker)

at this moment the differences are really small, cosmetic :)

@rudzikdawid
Copy link
Contributor

rudzikdawid commented Apr 7, 2018

background-500-0.18 has perfect match with master rgb(238,238,238)

0.18 background: rgba(158,158,158,0.18); === background: rgb(238,238,238);

I think we should use it.

@Splaktar
Copy link
Contributor Author

Splaktar commented Apr 8, 2018

Looks good. 0.18 it is ✅

@Splaktar Splaktar force-pushed the fixSelectPopupTheme branch from 9c19496 to 1400bc4 Compare April 8, 2018 05:37
@jelbourn jelbourn assigned tinayuangao and unassigned jelbourn Apr 9, 2018
@Splaktar Splaktar assigned mmalerba and andrewseguin and unassigned tinayuangao Apr 16, 2018
@andrewseguin andrewseguin merged commit f926c96 into master Apr 23, 2018
@mmalerba mmalerba removed their assignment Apr 23, 2018
@Splaktar Splaktar deleted the fixSelectPopupTheme branch April 24, 2018 07:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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: merge ready This PR is ready for a caretaker to review type: bug ui: theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

select: dropdown doesn't inherit dark theme
8 participants