feat(theme): Add new mdc-theme-on-primary global variable #2483

lynnmercier · 2018-03-28T20:26:06Z

BREAKING CHANGE: Removes the --mdc-theme-text-<TEXT_STYLE>-on-<THEME_COLOR> CSS custom properties, and the mdc-theme--text-<TEXT_STYLE>-on-<THEME_COLOR> CSS classes

codecov-io · 2018-03-28T20:36:26Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #2483   +/-   ##
=======================================
  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 82cd6ff...08a752f. Read the comment docs.

kfranqueiro · 2018-03-28T21:16:32Z

packages/mdc-theme/_variables.scss

@@ -23,11 +23,13 @@
 //

 $mdc-theme-primary: #6200ee !default; // baseline purple, 500 tone
+$mdc-theme-on-primary: #fff !default;


Shouldn't these still be computed based on $mdc-theme-primary or $mdc-theme-secondary?

i.e. these values should adjust to remain correct when the corresponding variable is changed, which we were doing before and should still easily be able to do by calling a mixin here.

Also, are we really doing away with semitransparent text OOTB? I thought that was in spec. I thought on-primary/secondary were additional slots, not replacing text?

mdc-theme should not compute $mdc-theme-on-primary based on $mdc-theme-primary because then $mdc-theme-on-primary is always white/black. Mdc-theme should allow users to customize $mdc-theme-on-primary to any color, like the brown color used in the shrine example.

Sorry if I worded poorly; my suggestion wouldn't prevent users from customizing it, it would just provide a sane default so that things don't get weird if you change e.g. primary and not on-primary.

We'd still have !default at the end to allow overriding it.

I cant actually figure how to build this functionality (without probably restructuring all our mixins in mdc-theme). I tried to do this

$mdc-theme-on-primary: #fff !default; @if mdc-theme-contrast-tone($mdc-theme-primary) == "dark" { $mdc-theme-on-primary: #000 !default; }

and commented out the $mdc-theme-on-primary in theme-shrine.scss...but it keeps setting the -on-primary color the white

That will fail because the earlier assignment will always happen, and then the latter one inside the if will never happen because !default makes the assignment only happen if that variable doesn't already have an assigned value. This is the behavior that makes redefining our globals before loading our styles work.

http://sass-lang.com/documentation/file.SASS_REFERENCE.html#variable_defaults_default

There is an if function that can be used akin to a ternary operator in other languages which you can use to set the value in one statement:

$mdc-theme-on-primary: if(mdc-theme-contrast-tone($mdc-theme-primary) == "dark", #000, #fff) !default;

Or, you can populate a map with light and dark entries and simply reference that with the value returned from contrast-tone. That is what the variables that you're replacing were already doing, which makes it hard for me to understand why we're removing the logic.

https://github.com/material-components/material-components-web/blob/v0.33.0/packages/mdc-theme/_variables.scss#L42-L63

How would users override the -on-primary with Sass without the changes in this PR?

IIUC your point is right now it's impossible to directly override, say, text-primary-on-primary, which is true. If enabling that is the goal, then I can see why this PR is changing how the variables are defined, so they can each be directly overridden.

I was specifically concerned about the existing dark/light logic that was being taken out of the picture in the process of this change, and was a little confused since in this thread you seemed to be having trouble replicating the logic that already exists.

How many text color levels are we going to end up with after this is all done (compared to the current primary/secondary/hint/disabled/icon)? Would it be cleaner to reuse the existing functions that pick the defaults out of the mdc-theme-text-colors map, versus writing ternaries with hard-coded values on every variable we declare?

Either way, we should be able to ensure sane behavior if someone doesn't touch the on-x variables, but does touch the primary/secondary values.

kfranqueiro · 2018-03-30T21:25:01Z

Removing the entries from the map causes CSS classes to no longer be generated here:

https://github.com/material-components/material-components-web/blob/v0.33.0/packages/mdc-theme/mdc-theme.scss#L30-L36

Which causes regressions in demo pages that use mdc-theme--x classes to apply color.

Are we expecting to maintain classes for this as before (e.g. now we would have mdc-theme--on-primary and --on-secondary)? We'll need to enumerate the new classes to generate in mdc-theme.scss.

Here's an example of a regression, in the theme page (the 3rd and 4th buttons should have white text):

kfranqueiro

There are other existing references to text-...-on-primary...

demos/icon-toggle.html (uses CSS classes)
demos/top-app-bar.html (uses CSS classes)
demos/drawer/temporary-drawer.html (uses CSS classes)
docs/theming.md (documents CSS classes and custom properties, and mentions it in prose and HTML/CSS examples)
The grid-list README mentions that it uses text-primary-on-primary

kfranqueiro · 2018-04-03T14:53:30Z

packages/mdc-theme/README.md

-> `$mdc-theme-secondary` instead.
-
-The text color, for text placed on top of these selected theme colors, is programmatically computed based on color contrast. We follow the Web Content Accessibility Guidelines 2.0.
+We suggest you follow the Web Content Accessibility Guidelines 2.0 we picking the values for on-primary, on-secondary, etc. These values should be accessible on top of the corresponding value, e.g. primary and secondar.


Typos:
"we picking" -> "when picking"
"secondar." -> "secondary."

kfranqueiro · 2018-04-03T14:54:06Z

packages/mdc-theme/README.md


 https://www.w3.org/TR/WCAG20

+### Advanced customization
+
+Color scheme will only get your app 80% of the way to a well designed app. Inevitably there will be some components that do not work "out of the box". To fix problems with accessbility and design, we suggest you use our SASS mixins, such as `mdc-button-filled-accessible`. For more information, consult the documentation for each component.


"your app" -> "you" (it's currently redundant of "well designed app" later in the sentence)
"well designed" -> "well-designed"
Typo: accessbility -> accessibility
SASS -> Sass (it's how they themselves capitalize it)

kfranqueiro · 2018-04-03T14:55:47Z

packages/mdc-theme/README.md

+
+### Non-Sass customization
+
+Only a very limited number of Material Design color customization features are supported for non-Sass clients. They are a set of CSS custom properties, and a set of CSS classes


Add period at end of sentence

kfranqueiro · 2018-04-03T14:58:02Z

packages/mdc-theme/_variables.scss

-  text-hint-on-secondary: mdc-theme-ink-color-for-fill_(hint, $mdc-theme-secondary),
-  text-disabled-on-secondary: mdc-theme-ink-color-for-fill_(disabled, $mdc-theme-secondary),
-  text-icon-on-secondary: mdc-theme-ink-color-for-fill_(icon, $mdc-theme-secondary),
+  on-primary: $mdc-theme-on-primary,


Just to confirm... are we expecting to update/prune the rest of this map (and the readme) in other imminent PRs?

…r does not specify on-primary

kfranqueiro

There is still a mention of text-primary-on-primary in the grid-list README.

Note: This will need BREAKING CHANGE to note the replacement of text-x-on-primary/secondary in both Sass and CSS variables, and the removal of text-hint/disabled/icon-on-primary-secondary. (Probably just need a general note that supported Sass and CSS variables have been renamed/removed, see readme for details.)

kfranqueiro · 2018-04-16T16:36:05Z

demos/theme/theme-shrine.scss

-// TODO: Use theme mixin(s) instead
-$mdc-theme-primary: #fff;
-$mdc-theme-secondary: $material-color-orange-500;
+$mdc-theme-primary: #FCB8AB;


Nit: uppercase hex to lowercase hex wherever these are used, to match other styles

kfranqueiro

Remember BREAKING CHANGE

kfranqueiro suggested changes Mar 28, 2018

View reviewed changes

kfranqueiro suggested changes Apr 3, 2018

View reviewed changes

lynnmercier and others added 4 commits April 6, 2018 13:02

feat(theme): Add new mdc-theme-on-primary global variable

2b6c37f

add the feature of picking black/white text for on-primary if the use…

b67d378

…r does not specify on-primary

fix theme demo page, update README

663bf91

feat(theme): Update for comments

6a0c403

williamernest force-pushed the feat/on-primary branch from ea4f009 to 6a0c403 Compare April 6, 2018 21:29

williamernest and others added 2 commits April 9, 2018 12:57

Merge branch 'master' into feat/on-primary

a54bea7

Merge branch 'master' into feat/on-primary

531eeaa

kfranqueiro suggested changes Apr 16, 2018

View reviewed changes

kfranqueiro force-pushed the master branch from 41f496c to 6f97632 Compare April 17, 2018 21:22

williamernest and others added 2 commits April 18, 2018 14:34

Merge branch 'master' into feat/on-primary

e62be16

feat(theme): Update for comments

38e155e

williamernest self-assigned this Apr 18, 2018

feat(theme): Update for comments

08a752f

kfranqueiro approved these changes Apr 18, 2018

View reviewed changes

williamernest merged commit 777a0fd into master Apr 18, 2018

williamernest deleted the feat/on-primary branch April 18, 2018 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(theme): Add new mdc-theme-on-primary global variable #2483

feat(theme): Add new mdc-theme-on-primary global variable #2483

lynnmercier commented Mar 28, 2018

codecov-io commented Mar 28, 2018 •

edited

Loading

kfranqueiro Mar 28, 2018

lynnmercier Mar 28, 2018

kfranqueiro Mar 28, 2018 •

edited

Loading

lynnmercier Mar 29, 2018

kfranqueiro Mar 30, 2018 •

edited

Loading

lynnmercier Mar 30, 2018

kfranqueiro Mar 30, 2018

kfranqueiro commented Mar 30, 2018

kfranqueiro left a comment

kfranqueiro Apr 3, 2018

kfranqueiro Apr 3, 2018

kfranqueiro Apr 3, 2018

kfranqueiro Apr 3, 2018

kfranqueiro left a comment

kfranqueiro Apr 16, 2018

kfranqueiro left a comment


		### Non-Sass customization

		Only a very limited number of Material Design color customization features are supported for non-Sass clients. They are a set of CSS custom properties, and a set of CSS classes

feat(theme): Add new mdc-theme-on-primary global variable #2483

feat(theme): Add new mdc-theme-on-primary global variable #2483

Conversation

lynnmercier commented Mar 28, 2018

codecov-io commented Mar 28, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro commented Mar 30, 2018

kfranqueiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 28, 2018 •

edited

Loading

kfranqueiro Mar 28, 2018 •

edited

Loading

kfranqueiro Mar 30, 2018 •

edited

Loading