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.

Sign up for GitHub

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

Jump to bottom

feat(ripple): Add new states mixins #1624

Merged
merged 5 commits into from
Nov 30, 2017
Merged

feat(ripple): Add new states mixins #1624

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
857bd92
feat(ripple): Add new states mixins
Nov 21, 2017
b8b7113
WIP Add fallback for keyframe opacity vars
Nov 28, 2017
6b600e0
WIP Address feedback RE naming, re-word docs, and remember to pass th…
Nov 29, 2017
d15527e
WIP I am a donut
Nov 29, 2017
9a68fdf
Merge branch 'master' into feat/states-mixins
kfranqueiro Nov 30, 2017
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
4 changes: 2 additions & 2 deletions packages/mdc-ripple/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,15 @@ Mixin | Description

Mixin | Description
--- | ---
`mdc-states-color($color, $include-focus-within)` | Adds state and ripple styles for the indicated color, deciding opacities based on whether the passed color is light or dark. Optionally includes `::focus-within` selector for focused state of CSS-only components.
`mdc-states($color, $has-nested-focusable-element)` | Adds state and ripple styles for the indicated color, deciding opacities based on whether the passed color is light or dark. `$has-nested-focusable-element` defaults to `false` but should be set to `true` if the component contains a focusable element (e.g. an input) under the root node.

##### Advanced States Mixins

Mixin | Description
--- | ---
`mdc-states-base-color($color)` | Sets up base state styles using the provided color
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think I understand the difference between mdc-states-color and mdc-states-base-color.

`mdc-states-hover-opacity($opacity)` | Adds styles for hover state using the provided opacity
`mdc-states-focus-opacity($opacity, $include-focus-within)` | Adds styles for focus state using the provided opacity, optionally including `::focus-within` selector for focused state of CSS-only components (useful when an element under the root node receives focus)
`mdc-states-focus-opacity($opacity, $has-nested-focusable-element)` | Adds styles for focus state using the provided opacity. `$has-nested-focusable-element` defaults to `false` but should be set to `true` if the component contains a focusable element (e.g. an input) under the root node.
`mdc-states-press-opacity($opacity)` | Adds styles for press state using the provided opacity

#### Legacy Sass API
Expand Down
8 changes: 4 additions & 4 deletions packages/mdc-ripple/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,14 +116,14 @@ $mdc-ripple-common-styles-emitted_: false !default;
}
}

@mixin mdc-states-focus-opacity($opacity, $include-focus-within: false) {
@mixin mdc-states-focus-opacity($opacity, $has-nested-focusable-element: false) {
// Focus overrides hover by reusing the ::before pseudo-element.
// :focus-within generally works on non-MS browsers and matches when a *child* of the element has focus.
// It is useful for cases where a component has a focusable element within the root node, e.g. text field,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the argument should be $focusable-element-within-root-element?

// but undesirable in general in case of nested stateful components.
// We use a modifier class for JS-enabled surfaces to support all use cases in all browsers.
$cssOnlyFocusSelector: if(
$include-focus-within,
$has-nested-focusable-element,
"&:not(.mdc-ripple-upgraded):focus::before, &:not(.mdc-ripple-upgraded):focus-within::before",
"&:not(.mdc-ripple-upgraded):focus::before"
);
Expand Down Expand Up @@ -157,7 +157,7 @@ $mdc-ripple-common-styles-emitted_: false !default;
}

// Simple mixin which automatically selects opacity values based on whether the ink color is light or dark.
@mixin mdc-states-color($color: black, $include-focus-within: false) {
@mixin mdc-states($color: black, $has-nested-focusable-element: false) {
$color-value: mdc-theme-prop-value($color);
$opacity-map: if(
mdc-theme-tone($color-value) == "light",
Expand All @@ -167,7 +167,7 @@ $mdc-ripple-common-styles-emitted_: false !default;

@include mdc-states-base-color($color);
@include mdc-states-hover-opacity(map-get($opacity-map, "hover"));
@include mdc-states-focus-opacity(map-get($opacity-map, "focus"));
@include mdc-states-focus-opacity(map-get($opacity-map, "focus"), $has-nested-focusable-element);
@include mdc-states-press-opacity(map-get($opacity-map, "press"));
}

Expand Down
6 changes: 3 additions & 3 deletions packages/mdc-ripple/mdc-ripple.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

.mdc-ripple-surface {
@include mdc-ripple-surface;
@include mdc-states-color;
@include mdc-states;
@include mdc-ripple-radius;

position: relative;
Expand All @@ -35,11 +35,11 @@
}

&--primary {
@include mdc-states-color(primary);
@include mdc-states(primary, true);
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 use a named param for true? E.g.:

@include mdc-states(primary, $has-nested-focusable-element: true);

At some point I'd like to go through all of our Sass mixins and use named params for all non-obvious boolean values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was actually just for testing and I forgot to remove it... and I sort of intentionally broke exactly this rule to try to remind myself to remove it... but instead you reminded me!

}

&--accent {
@include mdc-states-color(secondary);
@include mdc-states(secondary);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does --primary set $has-nested-focusable-element: true but --accent does not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I'm dumb and forgot to remove something I put in simply to verify that the mixin worked as intended (since the only component that will actually use that parameter is text field). :D Fixed.

}
}

Expand Down