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

fix(autocomplete): autocomplete panel theme supports dark mode#11203

Merged
tinayuangao merged 1 commit into
angular:masterfrom
rudzikdawid:fixAutocompletePopupTheme
Apr 10, 2018
Merged

fix(autocomplete): autocomplete panel theme supports dark mode#11203
tinayuangao merged 1 commit into
angular:masterfrom
rudzikdawid:fixAutocompletePopupTheme

Conversation

@rudzikdawid
Copy link
Copy Markdown
Contributor

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

Issue Number:
#11202

What is the new behavior?

The autocomplete 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

Before:

After:

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Mar 30, 2018
@rudzikdawid rudzikdawid force-pushed the fixAutocompletePopupTheme branch from b3296b8 to a1bee77 Compare March 30, 2018 21:22
@rudzikdawid rudzikdawid changed the title Fix autocomplete popup theme fix(autocomplete): autocomplete panel theme supports dark mode Mar 31, 2018
@Splaktar Splaktar added this to the 1.1.9 milestone Apr 5, 2018
@Splaktar Splaktar self-assigned this Apr 5, 2018
@Splaktar Splaktar self-requested a review April 5, 2018 02:41
@Splaktar Splaktar added type: bug ui: theme in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs ui: CSS P4: minor Minor issues. May not be fixed without community contributions. labels Apr 5, 2018
@Splaktar
Copy link
Copy Markdown
Contributor

Splaktar commented Apr 5, 2018

The focus background color in light mode has gotten significantly lighter. It should stay the same or at minimum be visually indistinguishable.

Before:
screen shot 2018-04-04 at 10 45 43 pm
After:
screen shot 2018-04-04 at 10 45 55 pm

Before:
screen shot 2018-04-04 at 10 46 48 pm
After:
screen shot 2018-04-04 at 10 47 03 pm

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.

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

@rudzikdawid
Copy link
Copy Markdown
Contributor Author

rudzikdawid commented Apr 5, 2018

You have right
hover background: '{{background-300-0.2}}'; wasn't perfect

so i tried your solution from #11198
but then i realized that background: '{{background-hue-2}}' also have problems, mostly in dark mode

after that i tried with hover background: '{{background-500-0.17}}';
and it seems ok

before:

after:

dark mode:

you can check it again, i made git push --force with fixes

@rudzikdawid rudzikdawid force-pushed the fixAutocompletePopupTheme branch from 5d43eef to dd28d4a Compare April 5, 2018 20:13
@Splaktar
Copy link
Copy Markdown
Contributor

Splaktar commented Apr 7, 2018

Thanks for the update. I went with background-500-0.20 after some testing in the select PR. Does it make sense to use that here as well?

@rudzikdawid
Copy link
Copy Markdown
Contributor Author

rudzikdawid commented Apr 7, 2018

I think that background-500-0.20 seems to be a little bit darker than master build, esspecially in light theme. background-500-0.18 looks mutch better.

master build https://material.angularjs.org/latest/demo/autocomplete:

.md-autocomplete-suggestions-container li:hover {
    background: rgb(238,238,238);
}

hover changes with 0.18:

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

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

result of convert:
0.18 background: rgba(158,158,158,0.18); === background: rgb(238,238,238);
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.18 no difference, perfect match
0.20 differ by 2 point (darker)

at this moment the differences are really small, cosmetic :)
I think now It does not matter which value we choose. Both values are much better than my first proposal background-300-0.2

@Splaktar
Copy link
Copy Markdown
Contributor

Splaktar commented Apr 8, 2018

Yep, as mentioned in #11198 (comment), let's use 0.18.

@rudzikdawid rudzikdawid force-pushed the fixAutocompletePopupTheme branch from dd28d4a to 1d80633 Compare April 8, 2018 07:38
@rudzikdawid
Copy link
Copy Markdown
Contributor Author

done

@Splaktar Splaktar added the needs: manual testing This issue or PR needs to have some manual testing and verification done label Apr 8, 2018
@Splaktar
Copy link
Copy Markdown
Contributor

Splaktar commented Apr 9, 2018

According to https://material.io/guidelines/components/text-fields.html#text-fields-states, it appears that a disabled dark mode text field (md-autocomplete with md-floating-label) should be have the default background color (i.e. the same as the content area). This seems to be displayed properly.

According to https://material.io/guidelines/components/text-fields.html#text-fields-text-field-boxes (I think that this maps closest to our autocomplete design w/o md-floating-label), it appears that the disabled dark mode should have a background color (#262626) that is slightly lighter than the default background (#1D1D1D) and slightly darker than the non-disabled background (#2F2F2F). Currently in this branch, it is darker than the default background in dark mode.
screen shot 2018-04-09 at 4 41 17 am

@Splaktar
Copy link
Copy Markdown
Contributor

Splaktar commented Apr 9, 2018

I don't see any combination that we have available atm that meets the requirements listed in my last post. I think that we should go with the following for the time being:

  &[disabled]:not([md-floating-label]) {
    background: '{{background-default}}';
  }

It still provides a difference between enabled/disabled styling, but avoids applying a darker background color than the default background.
screen shot 2018-04-09 at 5 02 31 am

@Splaktar
Copy link
Copy Markdown
Contributor

Splaktar commented Apr 9, 2018

But then again, testing light mode background-hue-2 gives us the same styling as previously, so if we use the background-default, it could cause problems. So let's keep it like you have it with background-hue-2 as I would rather have the inconsistency on the dark mode side rather than light mode.

@rudzikdawid
Copy link
Copy Markdown
Contributor Author

rudzikdawid commented Apr 9, 2018

i see this issue also on master build. So maybe we should open another issue/PR for that.
This issue #11202 was opened to prepare fix for dark theme.

@Splaktar Splaktar removed the needs: manual testing This issue or PR needs to have some manual testing and verification done label Apr 9, 2018
@Splaktar
Copy link
Copy Markdown
Contributor

Splaktar commented Apr 9, 2018

I think that we can send this for presubmit testing now.

@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 9, 2018
@rudzikdawid rudzikdawid closed this Apr 9, 2018
@rudzikdawid rudzikdawid reopened this Apr 9, 2018
@Splaktar Splaktar assigned jelbourn and unassigned Splaktar Apr 9, 2018
@jelbourn jelbourn assigned tinayuangao and unassigned jelbourn Apr 9, 2018
@tinayuangao tinayuangao merged commit aba7b2b into angular:master Apr 10, 2018
chmelevskij pushed a commit to chmelevskij/material that referenced this pull request Jun 19, 2018
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: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review type: bug ui: CSS ui: theme

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants