-
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(card): Add primary action element with hover, focused, and pressed states #2039
Conversation
BREAKING CHANGE: The `shouldBubble` param of `MDCComponent.emit()` now defaults to `true` instead of `false`.
This reverts commit 4c08741.
Codecov Report
@@ Coverage Diff @@
## master #2039 +/- ##
=======================================
Coverage 99.14% 99.14%
=======================================
Files 90 90
Lines 3843 3843
Branches 497 497
=======================================
Hits 3810 3810
Misses 33 33 Continue to review full report at Codecov.
|
# Conflicts: # demos/card.html # demos/card.scss # packages/mdc-card/README.md # packages/mdc-card/_mixins.scss # packages/mdc-card/mdc-card.scss
# Conflicts: # packages/mdc-card/README.md
mdc-card__primary-action
with statesThere 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.
I'm not seeing the hover/focus wash overlay the images in the last 2 demos. I thought I recalled this being something you had to fix in mdc-ripple, did we not do that?
packages/mdc-card/mdc-card.scss
Outdated
@include mdc-states; | ||
|
||
box-sizing: border-box; | ||
outline: 0 none; |
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.
Isn't the 0
here redundant?
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.
Done
packages/mdc-card/README.md
Outdated
@@ -172,6 +172,7 @@ CSS Class | Description | |||
--- | --- | |||
`mdc-card` | Mandatory, for the card element | |||
`mdc-card--stroked` | Removes the shadow and displays a hairline stroke instead | |||
`mdc-card__primary-action` | The main tappable area of the card. Typically contains most (or all) card content _except_ `mdc-card__actions`. |
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.
Might want to further qualify that this is only applicable to cards that have a primary action that the main surface should trigger?
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.
Done
packages/mdc-card/mdc-card.scss
Outdated
@@ -48,7 +48,9 @@ | |||
.mdc-card__media { | |||
@include mdc-card-media-aspect-ratio-base_; | |||
|
|||
display: flex; |
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.
I'm not sure this makes much sense here, but also it seems like it would make more sense (and also work) if we added display / flex-direction to .mdc-card__primary-action
instead.
In fact the 3 styles (not counting mixins) currently applied to mdc-card
should maybe just be put in a selector that applies to both mdc-card
and mdc-card__primary-action
?
`.mdc-button` sets a `min-width`, but `mdc-icon-toggle` does not. The `min-width: 0` was originally to override the `mdc-button` style, but that creates bad UX because it makes the tap target too small if the text is short (e.g., an "OK" button).
Fixes #1709