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(select): Move colors for default select to mixins #1934

Merged
merged 13 commits into from
Jan 17, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion demos/select.html
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ <h2>Select Multiple - CSS Only</h2>
}
});
alternateColorsCb.addEventListener('change', function() {
boxDemoWrapper.classList[alternateColorsCb.checked ? 'add' : 'remove']('demo-select-custom-colors');
root.classList[alternateColorsCb.checked ? 'add' : 'remove']('demo-select-custom-colors');
});
disabledCb.addEventListener('change', function() {
select.disabled = disabledCb.checked;
Expand Down
2 changes: 1 addition & 1 deletion demos/select.scss
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

// stylelint-disable selector-class-pattern
.demo-select-custom-colors {
@include mdc-select-fill-color(rgba(blue, .1));
@include mdc-select-container-fill-color(rgba(blue, .1));
@include mdc-select-ink-color(blue);
@include mdc-select-label-color(rgba(blue, .6));
@include mdc-select-bottom-line-color(rgba(blue, .5));
Expand Down
6 changes: 2 additions & 4 deletions packages/mdc-select/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -276,16 +276,14 @@ 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.*
`mdc-select-fill-color($color)` | Customizes the background color of the select.
`mdc-select-ink-color($color)` | Customizes the text entered into the select. This style applies only to the css version.
`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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

`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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@williamernest williamernest Jan 11, 2018

Choose a reason for hiding this comment

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

screen shot 2018-01-11 at 3 40 38 pm

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


>_NOTE_: * - This style applies to the css only version.

### MDC Select Component API

The MDC Select component API is modeled after a subset of the `HTMLSelectElement` functionality, and
Expand Down
23 changes: 15 additions & 8 deletions packages/mdc-select/_mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@
// Public

@mixin mdc-select-ink-color($color) {
:not(.mdc-select--disabled) {
&:not(.mdc-select--disabled) {
@include mdc-select-ink-color_($color);
}
}

@mixin mdc-select-fill-color($color) {
:not(.mdc-select--disabled) {
@include mdc-select-fill-color_($color);
@mixin mdc-select-container-fill-color($color) {
&:not(.mdc-select--disabled) {
@include mdc-select-container-fill-color_($color);
}
}

Expand Down Expand Up @@ -58,6 +58,14 @@
}
}

@mixin mdc-select-focused-bottom-line {
Copy link
Contributor

Choose a reason for hiding this comment

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

you should make this private

// 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;
}
}

// Private

@mixin mdc-select-ink-color_($color) {
Expand All @@ -66,7 +74,7 @@
}
}

@mixin mdc-select-fill-color_($color) {
@mixin mdc-select-container-fill-color_($color) {
.mdc-select__surface {
@include mdc-theme-prop(background-color, $color);
}
Expand Down Expand Up @@ -98,12 +106,11 @@
}

@mixin mdc-select-focused-bottom-line-color_($color) {
.mdc-select__surface:focus .mdc-select__bottom-line,
.mdc-select__surface:focus ~ .mdc-select__bottom-line {
@include mdc-select-focused-bottom-line {
@include mdc-theme-prop(background-color, $color);
}

.mdc-select--open .mdc-select__bottom-line {
&.mdc-select--open .mdc-select__bottom-line {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@williamernest williamernest Jan 12, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@include mdc-theme-prop(background-color, $color);
}
}
Expand Down
13 changes: 4 additions & 9 deletions packages/mdc-select/mdc-select.scss
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,18 @@ $mdc-select-arrow-padding: 26px;
$mdc-select-label-padding: 16px;
$mdc-select-menu-transition: transform 180ms $mdc-animation-standard-curve-timing-function;

// postcss-bem-linter: define select
.mdc-select {
@include mdc-select-dd-arrow-svg-bg_;
@include mdc-select-fill-color(rgba(white, .1));
@include mdc-select-ink-color(primary);
@include mdc-select-container-fill-color(rgba(white, .1));
@include mdc-select-ink-color(text-primary-on-light);
@include mdc-select-label-color(rgba(black, .6));
@include mdc-select-bottom-line-color(rgba(black, .5));
@include mdc-select-selected-text-color(text-primary-on-light);

// Focused state colors
@include mdc-select-focused-bottom-line-color(primary);
@include mdc-select-focused-label-color(primary);
}

// postcss-bem-linter: define select
.mdc-select {
@include mdc-typography(subheading2);

display: inline-flex;
Expand Down Expand Up @@ -160,9 +157,7 @@ $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 {
@include mdc-select-focused-bottom-line {
transform: scaleY(2);

&::after {
Expand Down