-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(fab): Add mdc-fab-accessible mixin #1238
Conversation
packages/mdc-fab/README.md
Outdated
|
||
#### `mdc-fab-accessible($container-color)` | ||
|
||
Changes the FAB's container color to the given color, and updates the FABs ink and ripple color to be meet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FABs -> FAB's; "to be meet" -> "to meet"
packages/mdc-fab/README.md
Outdated
Changes the FAB's container color to the given color, and updates the FABs ink and ripple color to be meet | ||
accessibility standards. | ||
|
||
The following mixins are intended for advanced users. These mixins will override the color of the container, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a horizontal rule above this paragraph (---
should do it)? I'm concerned otherwise that this will look like it belongs to the preceding heading rather than as a foreword to the next ones.
packages/mdc-fab/_mixins.scss
Outdated
@mixin mdc-fab-accessible($container-color) { | ||
@include mdc-fab-container-color($container-color); | ||
$light-or-dark: mdc-theme-light-or-dark($container-color); | ||
@if ($light-or-dark == 'light') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be helpful to have a comment here about how 'light'
refers to the text color rather than the background color... I had to look it up to convince myself this code wasn't backwards.
packages/mdc-fab/_variables.scss
Outdated
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
$light-ripple-config: (base-color: white, opacity: .16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline at end of file?
@@ -14,6 +14,7 @@ | |||
|
|||
@import "@material/theme/variables"; | |||
@import "./mixins"; | |||
@import "./variables"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually needed here, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, forgot to reference it. Updated now.
packages/mdc-fab/_variables.scss
Outdated
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
$light-ripple-config: (base-color: white, opacity: .16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we thinking this variable might eventually live in another package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably some day, but we can refactor state stuff then.
No description provided.