Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(color): Add on-surface and surface to theme. #2556

Merged
merged 20 commits into from
Apr 18, 2018

Conversation

williamernest
Copy link
Contributor

Fixes: #2482

@include mdc-card-corner-radius(2px);
@include mdc-elevation(2);
@include mdc-card-container-layout_;
}

.mdc-card--stroked {
@include mdc-elevation(0);
@include mdc-card-stroke(#dbdbdb);
@include mdc-card-stroke(rgba(mdc-theme-prop-value(on-surface), .12));
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make the stroke translucent, use the Sass mix() function instead:

mix(mdc-theme-prop-value(on-surface), mdc-theme-prop-value(surface), 12%)

Note: the mix() function takes percentages so make sure to use 12%, not .12

$mdc-chip-border-radius-default: 16px;
$mdc-chip-fill-color-default: rgba(black, .08);
$mdc-chip-ink-color-default: rgba(black, .6);
$mdc-chip-fill-color-default: rgba(mdc-theme-prop-value(on-surface), .12);
Copy link
Contributor

Choose a reason for hiding this comment

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

These should all use the mix() function too, but that can go in another PR (to fix #2479)

@@ -24,7 +24,7 @@

// Special case, so that .mdc-theme--background changes background color, not text color.
.mdc-theme--background {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look like it's being used or documented anywhere so we can probably get rid of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're keeping background. I reverted changes that I made to the components other than chips/card.

@codecov-io
Copy link

codecov-io commented Apr 13, 2018

Codecov Report

Merging #2556 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2556   +/-   ##
=======================================
  Coverage   98.68%   98.68%           
=======================================
  Files          98       98           
  Lines        4194     4194           
  Branches      533      533           
=======================================
  Hits         4139     4139           
  Misses         55       55

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 777a0fd...f60d563. Read the comment docs.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot
Copy link

CLAs look good, thanks!

@williamernest williamernest changed the base branch from feat/on-primary to master April 18, 2018 22:01
@@ -32,6 +32,9 @@ $mdc-theme-secondary: $mdc-theme-accent !default;
$mdc-theme-on-secondary: if(mdc-theme-contrast-tone($mdc-theme-secondary) == "dark", #000, #fff) !default;
$mdc-theme-background: #fff !default; // White

$mdc-theme-surface: white !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: change to #fff since we changed all the others to #fff

@include mdc-card-corner-radius(2px);
@include mdc-elevation(2);
@include mdc-card-container-layout_;
}

.mdc-card--stroked {
@include mdc-elevation(0);
@include mdc-card-stroke($mdc-card-stroke-color);
@include mdc-card-stroke(mix(mdc-theme-prop-value(on-surface), mdc-theme-prop-value(surface), 12%));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update the variable instead of replacing it here? (IIRC I added that variable for reuse with shape, at least in demos, but potentially of use to users for similar purposes as well)

Also FYI this results in changing from #dbdbdb to #e0e0e0

@williamernest williamernest merged commit 9639689 into master Apr 18, 2018
@williamernest williamernest deleted the feat/color/on-surface branch April 18, 2018 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants