fix(theming): light text on colored backgrounds should be opaque #7421

Maistho · 2017-09-29T17:13:34Z

According to Material Design Guidelines, white text on colored backgrounds should be displayed at an opacity of 100%.

I also changed the naming of the variables to [dark/light]-primary-text instead of [black/white]-87-opacity, since it better reflects the names used in the guidelines.

This PR fixes #6236

googlebot · 2017-09-29T17:13:37Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

Maistho · 2017-09-29T17:14:18Z

I signed it!

googlebot · 2017-09-29T17:14:20Z

CLAs look good, thanks!

jelbourn · 2017-10-06T14:38:05Z

@josephperrott can you take a look to see how this compares to the spec / mdc-web?

josephperrott · 2017-10-06T15:35:32Z

@jelbourn
These changes match up with the values currently in the spec (under the heading Text on backgrounds)

MDC uses the same opacity in their theming

@Maistho Can you rebase the PR?

Maistho · 2017-10-06T17:17:27Z

@josephperrott I've rebased it!

josephperrott · 2017-10-10T17:27:05Z

@Maistho Looks like there are some build errors now?

Maistho · 2017-10-10T18:20:54Z

@josephperrott Sorry, I had apparently used $dark-disabled in some places and $dark-disabled-text in others.

Fixed it and rebased on master again.

jelbourn · 2017-10-11T21:09:28Z

src/lib/core/theming/_palette.scss

+$dark-disabled-text: rgba(black, 0.38);
+$dark-dividers: rgba(black, 0.12);
+$dark-focused: rgba(black, 0.12);
+$light-primary-text: rgba(white, 1);


rgba(white, 1) can just be white

I felt like it was more clear/explicit that the opacity should be 100% if I typed it out, but I can change it if you prefer it.

Yeah, I prefer to use the plain value when it is desired

Fixed and rebased

jelbourn · 2017-10-12T22:10:18Z

Caretaker note: this PR will likely affect many screenshot tests and should be synced by itself. Reach out to teams to ensure that everything still looks correct.

Maistho · 2017-11-06T23:50:11Z

Hi, what's the status on this PR? Anything I can do to help speed things up?

jelbourn · 2017-11-07T01:48:14Z

Our merge process is just a bit behind at the moment; see https://github.com/angular/material2/blob/master/CODE_REVIEWS.md#how-prs-are-merged for more context.

This change is particularly involved because it involves updating many screenshot tests.

jelbourn · 2017-11-19T00:36:19Z

@Maistho looks like there is a test failure on CI- can you rebase and see if it goes away?

jelbourn · 2017-11-19T04:30:22Z

@Maistho can you also restore the removed scss variables and mark them as deprecated? Upon running an internal presubmit it turns out quite a large number of apps consume on them.

Maistho · 2017-11-19T18:53:15Z

@jelbourn I've rebased it!

How do I add the deprecation notice? Not sure if you use sassdoc, if I should add it to the commit message, or if there's some other way.

I added sassdoc compatible comments for now.

jelbourn · 2017-11-21T05:19:02Z

Looks good to me

andrewseguin · 2017-12-13T21:16:37Z

@jelbourn Tests pass - do you consider this a patch or minor

Edit: Re-running internal tests, looks like a lot of them happened to fall through when skipping already failing tests

jelbourn · 2017-12-13T21:52:49Z

I would consider this acceptable for a patch because it only changes colors

andrewseguin · 2018-01-11T18:41:58Z

@Maistho Thanks for your patience as we try and get this merged in. We're seeing some screenshot diffs get brought up that might point out something incorrect in this PR.

Can you look at these screenshots and confirm the appearance of the disabled button? Looks like your change made it darker which in my eyes doesn't match the spec.

Spec

Master (demo app, button page)

pr/4721 (demo app, button page)

Maistho · 2018-01-11T19:05:07Z

@andrewseguin Yup, I made a mistake there. Looks like the disabled buttons are using the text color for background. I'll take a look!

According to [Material Design Guidelines](https://material.io/guidelines/style/color.html#color-usability) white text on colored backgrounds should do so at an opacity of 100%

Maistho · 2018-01-11T19:33:29Z

@andrewseguin rebased and pushed a fixed version

…ular#7421) According to [Material Design Guidelines](https://material.io/guidelines/style/color.html#color-usability) white text on colored backgrounds should do so at an opacity of 100%

According to [Material Design Guidelines](https://material.io/guidelines/style/color.html#color-usability) white text on colored backgrounds should do so at an opacity of 100%

angular-automatic-lock-bot · 2019-09-08T02:01:04Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

Maistho requested review from crisbeto, jelbourn and josephperrott as code owners September 29, 2017 17:13

googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Sep 29, 2017

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Sep 29, 2017

jelbourn added P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful pr: needs review labels Oct 6, 2017

jelbourn reviewed Oct 11, 2017

View reviewed changes

jelbourn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Oct 12, 2017

crisbeto mentioned this pull request Oct 14, 2017

fix(select,input): inconsistent disabled text color #7794

Merged

jelbourn added P2 The issue is important to a large percentage of users, with a workaround and removed P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful labels Oct 26, 2017

jelbourn added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Nov 19, 2017

jelbourn removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Nov 21, 2017

andrewseguin added presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged target: patch This PR is targeted for the next patch release labels Dec 13, 2017

fix(theming): light text on colored backgrounds should be opaque

3560c08

According to [Material Design Guidelines](https://material.io/guidelines/style/color.html#color-usability) white text on colored backgrounds should do so at an opacity of 100%

andrewseguin merged commit 1701b98 into angular:master Jan 12, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(theming): light text on colored backgrounds should be opaque #7421

fix(theming): light text on colored backgrounds should be opaque #7421

Maistho commented Sep 29, 2017 •

edited

Loading

googlebot commented Sep 29, 2017

Maistho commented Sep 29, 2017

googlebot commented Sep 29, 2017

jelbourn commented Oct 6, 2017

josephperrott commented Oct 6, 2017

Maistho commented Oct 6, 2017

josephperrott commented Oct 10, 2017

Maistho commented Oct 10, 2017

jelbourn Oct 11, 2017

Maistho Oct 11, 2017

jelbourn Oct 12, 2017

Maistho Oct 12, 2017

jelbourn commented Oct 12, 2017

Maistho commented Nov 6, 2017

jelbourn commented Nov 7, 2017

jelbourn commented Nov 19, 2017

jelbourn commented Nov 19, 2017

Maistho commented Nov 19, 2017

jelbourn commented Nov 21, 2017

andrewseguin commented Dec 13, 2017 •

edited

Loading

jelbourn commented Dec 13, 2017

andrewseguin commented Jan 11, 2018

Maistho commented Jan 11, 2018

Maistho commented Jan 11, 2018

angular-automatic-lock-bot bot commented Sep 8, 2019

fix(theming): light text on colored backgrounds should be opaque #7421

fix(theming): light text on colored backgrounds should be opaque #7421

Conversation

Maistho commented Sep 29, 2017 • edited Loading

googlebot commented Sep 29, 2017

Maistho commented Sep 29, 2017

googlebot commented Sep 29, 2017

jelbourn commented Oct 6, 2017

josephperrott commented Oct 6, 2017

Maistho commented Oct 6, 2017

josephperrott commented Oct 10, 2017

Maistho commented Oct 10, 2017

jelbourn Oct 11, 2017

Choose a reason for hiding this comment

Maistho Oct 11, 2017

Choose a reason for hiding this comment

jelbourn Oct 12, 2017

Choose a reason for hiding this comment

Maistho Oct 12, 2017

Choose a reason for hiding this comment

jelbourn commented Oct 12, 2017

Maistho commented Nov 6, 2017

jelbourn commented Nov 7, 2017

jelbourn commented Nov 19, 2017

jelbourn commented Nov 19, 2017

Maistho commented Nov 19, 2017

jelbourn commented Nov 21, 2017

andrewseguin commented Dec 13, 2017 • edited Loading

jelbourn commented Dec 13, 2017

andrewseguin commented Jan 11, 2018

Maistho commented Jan 11, 2018

Maistho commented Jan 11, 2018

angular-automatic-lock-bot bot commented Sep 8, 2019

Maistho commented Sep 29, 2017 •

edited

Loading

andrewseguin commented Dec 13, 2017 •

edited

Loading