feat(select): Move colors for default select to mixins #1934

williamernest · 2018-01-10T22:19:31Z

Move colors for the select into a new mixins file.

codecov-io · 2018-01-10T22:55:47Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1934   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files          84       84           
  Lines        3718     3718           
  Branches      486      486           
=======================================
  Hits         3697     3697           
  Misses         21       21

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 568fc32...6bda358. Read the comment docs.

bonniezhou

Is dark theme to be removed in this PR? If so, we should remove the rest of the dark theme CSS. If not... maybe we should remove the "dark theme" option on the demo page anyways.

lynnmercier · 2018-01-11T22:20:39Z

packages/mdc-select/README.md

+Mixin | Description
+--- | ---
+`mdc-select-ink-color($color)` | Customizes the text entered into the select.* 
+`mdc-select-fill-color($color)` | Customizes the background color of the select. 


container-fill-color, not just fill-color. Its a little more specific, since just "fill" is vague

lynnmercier · 2018-01-11T23:02:13Z

packages/mdc-select/README.md

+`mdc-select-focused-bottom-line-color($color)` | Customizes the color of the bottom line of the select when focused.
+`mdc-select-selected-text-color($color)` | Customizes the color of the option that has been selected. 
+
+>_NOTE_: * - This style applies to the css only version.


this note seems unfinished?

It references the asterisk on line 279, the mdc-select-ink-color is only applied on the css only version. I'll put it in the description column.

lynnmercier · 2018-01-11T23:03:32Z

packages/mdc-select/_mixins.scss

+
+@mixin mdc-select-focused-bottom-line-color_($color) {
+  .mdc-select__surface:focus .mdc-select__bottom-line,
+  .mdc-select__surface:focus ~ .mdc-select__bottom-line {


why do you need both these selectors?

JS version has bottom line as a child element. CSS version is a sibling element.

lynnmercier · 2018-01-11T23:04:19Z

packages/mdc-select/mdc-select.scss

+  @include mdc-select-focused-bottom-line-color(primary);
+  @include mdc-select-focused-label-color(primary);
+}
+
 // postcss-bem-linter: define select
 .mdc-select {


combine this with the above mdc-select...and move the postcss-bem-linter up as well

lynnmercier · 2018-01-11T23:05:26Z

packages/mdc-select/mdc-select.scss

@@ -147,11 +138,8 @@ $mdc-select-menu-transition: transform 180ms $mdc-animation-standard-curve-timin
    transform: scaleY(1);
    transform-origin: bottom;
    transition: $mdc-select-menu-transition;
-    background-color: rgba(black, .5);

    &::after {


I dont see any ::after in the mixins...did we lose this logic?

On focus/open, the bottom-line turned the primary color. Then the ::after psuedo element applied a transition from the center moving outward that is also the primary color.

Purple is the surface:focus .mdc-select__bottom-line element.
Green is the ::after psuedo element animating outward.

Removing the entire ::after element felt like a separate PR.

lynnmercier · 2018-01-11T23:06:55Z

packages/mdc-select/mdc-select.scss

@@ -175,8 +163,6 @@ $mdc-select-menu-transition: transform 180ms $mdc-animation-standard-curve-timin
  // JS-enhanced and CSS-only Selects require different selectors for focused bottom-line due to different DOM structure
  &__surface:focus .mdc-select__bottom-line,
  &__surface:focus ~ .mdc-select__bottom-line {


ah...see my comment above where I was confused why we need this.

I would move this to another mixin like this

@mixin mdc-select-focused-bottom-line { // JS-enhanced and CSS-only Selects require different selectors for focused bottom-line due to different DOM structure .mdc-select__surface:focus .mdc-select__bottom-line, .mdc-select__surface:focus ~ .mdc-select__bottom-line { @content; } }

And then use that mixin here in mdc-select.scss and in mdc-select/_mixins.scss

lynnmercier · 2018-01-11T23:11:20Z

packages/mdc-select/README.md

+`mdc-select-focused-label-color($color, $opacity: 0.87)` | Customizes the label color of the select when focused. Changing opacity for the label when floating is optional.
+`mdc-select-bottom-line-color($color)` | Customizes the color of the default bottom line of the select.
+`mdc-select-focused-bottom-line-color($color)` | Customizes the color of the bottom line of the select when focused.
+`mdc-select-selected-text-color($color)` | Customizes the color of the option that has been selected. 


Can you attach a screenshot of what you are coloring here? I cant figure out what this is.

The "Fruit Roll Ups" text is the mdc-select-selected-text-color

Maybe mdc-select-selected-item-ink-color? The css class is .mcd-select__selected-text so I left it similar for the mixin.

Try and combine this with mdc-select-ink-color. And mdc-select-ink-color is the better name.

…te demo.

williamernest · 2018-01-12T17:06:00Z

@bonniezhou This PR doesn't remove dark theme, but knowing it's coming out soon I didn't take the time to add a dark theme.

lynnmercier · 2018-01-12T19:50:59Z

packages/mdc-select/_mixins.scss

@@ -58,6 +58,14 @@
  }
 }

+@mixin mdc-select-focused-bottom-line {


you should make this private

lynnmercier · 2018-01-12T19:51:54Z

packages/mdc-select/_mixins.scss

    @include mdc-theme-prop(background-color, $color);
  }

-  .mdc-select--open .mdc-select__bottom-line {
+  &.mdc-select--open .mdc-select__bottom-line {


why do you need this CSS selector? why is mdc-select-focused-bottom-line not enough?

mdc-select-focused-bottom-line only covers :focus. The component is no longer focused when the menu is open, the menu is focused. When the menu closes, the component regains :focus.

ahh...okay add a comment above this CSS selector

lynnmercier

LGTM!

lynnmercier · 2018-01-17T01:42:32Z

packages/mdc-select/README.md

@@ -276,13 +244,12 @@ these mixins within CSS selectors like `.foo-select` to apply styling.

 Mixin | Description
 --- | ---
-`mdc-select-ink-color($color)` | Customizes the text entered into the select. This style applies only to the css version.  
+`mdc-select-ink-color($color)` | Customizes the color of the selected item displayed in the select. On css version, this also customized the color of the label.
 `mdc-select-container-fill-color($color)` | Customizes the background color of the select. 
 `mdc-select-label-color($color)` | Customizes the label color of the select in the unfocused state. 


Add that this only covers the JS only version....otherwise label-color is covered by ink-color for CSS only

williamernest added 2 commits January 10, 2018 14:40

feat(select): Move colors to mixins

741870c

feat(select): Add demo. Update readme. Rename fields.

27b4aa8

williamernest force-pushed the feat/select/sass_color_mixins branch from eeb8c88 to 79bdf0f Compare January 10, 2018 22:41

feat(select): Fix readme.

cb5b822

williamernest force-pushed the feat/select/sass_color_mixins branch from 79bdf0f to cb5b822 Compare January 10, 2018 22:43

bonniezhou reviewed Jan 11, 2018

View reviewed changes

lynnmercier suggested changes Jan 11, 2018

View reviewed changes

lynnmercier self-assigned this Jan 11, 2018

williamernest and others added 3 commits January 11, 2018 18:02

Merge branch 'master' into feat/select/sass_color_mixins

06fddb4

feat(select): Update readme. Move focus bottom line color mixin. Upda…

219c2fa

…te demo.

feat(select): Combine mdc-select classes.

68bd036

lynnmercier suggested changes Jan 12, 2018

View reviewed changes

williamernest added 2 commits January 12, 2018 13:31

feat(select): Make bottom-line mixin private

8b92157

feat(select): Consolidate selected item color and ink color mixins

0b1296d

williamernest closed this Jan 12, 2018

williamernest deleted the feat/select/sass_color_mixins branch January 12, 2018 22:00

williamernest restored the feat/select/sass_color_mixins branch January 12, 2018 22:00

williamernest reopened this Jan 12, 2018

williamernest and others added 2 commits January 12, 2018 14:02

Merge branch 'master' into feat/select/sass_color_mixins

2ead80b

feat(select): Remove commented out code

8dc389d

lynnmercier approved these changes Jan 17, 2018

View reviewed changes

williamernest added 3 commits January 17, 2018 11:08

feat(select): Merge master

34be534

feat(select): Update transitions for bottom line after merge

f7fc139

feat(select): Make linter happy

6bda358

williamernest force-pushed the feat/select/sass_color_mixins branch from 41bb402 to 6bda358 Compare January 17, 2018 21:08

williamernest merged commit d6c68ce into master Jan 17, 2018

williamernest deleted the feat/select/sass_color_mixins branch January 17, 2018 21:22

williamernest mentioned this pull request Jan 31, 2018

Add Sass color mixin to select #1150

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(select): Move colors for default select to mixins #1934

feat(select): Move colors for default select to mixins #1934

williamernest commented Jan 10, 2018

codecov-io commented Jan 10, 2018 •

edited

bonniezhou left a comment

lynnmercier Jan 11, 2018

lynnmercier Jan 11, 2018

williamernest Jan 11, 2018

lynnmercier Jan 11, 2018

williamernest Jan 11, 2018

lynnmercier Jan 11, 2018

lynnmercier Jan 11, 2018

williamernest Jan 12, 2018 •

edited

lynnmercier Jan 11, 2018

lynnmercier Jan 11, 2018

williamernest Jan 11, 2018 •

edited

lynnmercier Jan 12, 2018

williamernest commented Jan 12, 2018

lynnmercier Jan 12, 2018

lynnmercier Jan 12, 2018

williamernest Jan 12, 2018 •

edited

lynnmercier Jan 17, 2018

lynnmercier left a comment

lynnmercier Jan 17, 2018

feat(select): Move colors for default select to mixins #1934

feat(select): Move colors for default select to mixins #1934

Conversation

williamernest commented Jan 10, 2018

codecov-io commented Jan 10, 2018 • edited

Codecov Report

bonniezhou 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamernest Jan 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamernest Jan 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamernest commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamernest Jan 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynnmercier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 10, 2018 •

edited

williamernest Jan 12, 2018 •

edited

williamernest Jan 11, 2018 •

edited

williamernest Jan 12, 2018 •

edited