feat(card): Add theme mixins; remove content layouts #2025
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 #2025 +/- ##
=======================================
Coverage 99.43% 99.43%
=======================================
Files 84 84
Lines 3720 3720
Branches 486 486
=======================================
Hits 3699 3699
Misses 21 21Continue to review full report at Codecov.
|
Will create a separate PR
demos/card.scss
Outdated
| @include mdc-ripple-radius; | ||
| @include mdc-states; | ||
|
|
||
| &:not([data-mdc-ripple-is-unbounded]) { |
There was a problem hiding this comment.
Would it make more sense to just apply these styles to .demo-card-article instead?
There was a problem hiding this comment.
Potentially, but if we ever decide to add another card example that uses
demo-card__ripple-surface on a different element, it won't get the benefit of the
IE/Edge workaround (position: relative).
| data-toggle-off='{"content": "favorite_border", "label": "Add to favorites"}'> | ||
| favorite_border | ||
| </i> | ||
| <i class="material-icons mdc-card__action mdc-card__action--icon mdc-ripple-surface" |
There was a problem hiding this comment.
Can we do something to make the padding of these consistent with the icon-toggle? They currently mismatch (and I'm fixing the unbounded ripple size in #2092 which will make the icon-toggle ripple the correct size).
This ensures that the tap area and ripple is the same size on both icon-toggle and non-icon-toggle actions.
Centered icons are not in spec
|
Good catch on the RTL corner radius. Fixed it in the demo. |
…ayouts # Conflicts: # demos/card.html
packages/mdc-card/_mixins.scss
Outdated
|
|
||
| @mixin mdc-card-box-sizing_ { | ||
| &, | ||
| &::before, |
There was a problem hiding this comment.
Do we really need the ::before and ::after selectors on every single thing this mixin is applied to? I would only expect it to be needed on before/after for __action and possibly card itself assuming we have ripples on the whole card at some point.
There was a problem hiding this comment.
There's no direct need for it in mdc-card ATM.
However, per #1455, I'd like to try to start consistently applying border-box to all of our elements (and pseudo elements) so that there are no more surprises (for us or our clients).
I can't tell you how many times I've gone to add padding to something in MDC Web and had it end up bigger than I expected because of stupid content-box. I want to get to a place where we can just assume that all of our elements have a sane box-sizing by default (i.e., border-box), unless we specifically override it for some reason.
WDYT?
There was a problem hiding this comment.
I guess, but most of our elements don't have ::before and ::after unless they have ripples, which is why I thought that in particular seemed excessive. Keep in mind how much CSS this is adding because we're including this mixin multiple times, and it's being expanded to a full selector.
Maybe add a parameter to this mixin to conditionalize the inclusion of before/after in the selector? (There's an example of how to conditionalize the selector in the focus state styles in mdc-ripple/_mixins)
There was a problem hiding this comment.
As discussed offline: Libraries/frameworks generally should not mess with ::before and ::after unless absolutely necessary (e.g., mdc-ripple). This is to prevent conflicts with other libraries and client code which might also set styles on the pseudos.
Removed the mdc-card-box-sizing_ mixin and directly applied box-sizing: border-box to each element individually.
packages/mdc-card/_mixins.scss
Outdated
| } | ||
| } | ||
|
|
||
| @mixin mdc-card-column-layout_($display: flex) { |
There was a problem hiding this comment.
What is the point of the display parameter existing if we never set it and we pretty much assume it's flex given the presence of flex-direction, anyway?
Actually, do we even need this mixin? AFAICT these styles don't affect anything on __media and __media-content so they're only actually needed once, on mdc-card...
There was a problem hiding this comment.
Removed! Good catch!
BREAKING CHANGE: All CSS classes for content layouts have been removed. Clients should decide what kind of layout is best for their specific use case. Dark theme CSS classes have been removed; use the Sass mixin or custom CSS instead.
Fixes #1126
Updated README to match best practices.
Removed classes (12)
mdc-card--theme-darkmdc-card__primarymdc-card__titlemdc-card__title--largemdc-card__subtitlemdc-card__supporting-textmdc-card__media-itemmdc-card__media-item--1dot5xmdc-card__media-item--2xmdc-card__media-item--3xmdc-card__actions--verticalmdc-card__horizontal-blockNew classes (8)
mdc-card__media--squaremdc-card__media--16-9mdc-card__media-contentmdc-card__actions--full-bleedmdc-card__action-buttonsmdc-card__action-iconsmdc-card__action--buttonmdc-card__action--iconNew mixins (3)
mdc-card-fill-color($color)mdc-card-corner-radius($radius)mdc-card-media-aspect-ratio($x, $y)Screenshot (edited)