prototype(card): create prototype card based on MDC web #16588
Conversation
jelbourn
added
pr: merge safe
target: patch
googlebot
added
the
cla: yes
jelbourn
force-pushed
the
mdc-card
branch
6 times, most recently
from
3d1d60f to
e8a3330
Compare
| deps = [ | ||
| "//src/material-experimental/mdc-helpers:mdc_helpers_scss_lib", | ||
| "//src/material-experimental/mdc-helpers:mdc_scss_deps_lib", | ||
| "//src/material/core:all_themes", |
There was a problem hiding this comment.
why do we bring in the whole theme here? shouldn't we just fine-grain what is used?
This generally causes unnecessary rebuilds if unrelated things change and the Sass compilation is not very fast right now (it's a bottleneck)
There was a problem hiding this comment.
Good call, I just copy-pasted from the checkbox without thinking about it. It's actually not necessary at all.
| // the title, and add a margin bottom to create space underneath the header. | ||
| .mat-mdc-card-subtitle { | ||
| margin-top: -8px; | ||
| margin-bottom: 16px; |
There was a problem hiding this comment.
Just asking out of curiosity: can't we calculate this somehow through variables? (it feels a bit like magic numbers to me)
There was a problem hiding this comment.
Changed to use $mat-card-default-padding
| } | ||
|
|
||
| // When the card actions come after the footer, eliminate the action's bottom padding because | ||
| // it's caputred by the footer. |
There was a problem hiding this comment.
| // it's caputred by the footer. | |
| // it's captured by the footer. |
There was a problem hiding this comment.
Also can you explain what is happening here? I'm confused by the comment that says that the footer captures the bottom padding of the actions? how can that work if the footer is an element before the actions?
There was a problem hiding this comment.
You're right, this makes no sense. Removed.
| width: $mat-card-header-size; | ||
| border-radius: 50%; | ||
| flex-shrink: 0; | ||
|
|
There was a problem hiding this comment.
Does this also need a background-position: center.
There was a problem hiding this comment.
The current mat-card-avatar doesn't have it, so I'm just leaving it the same
| // When both a title and a subtitle are used, eliminate the top padding of whichever comes second | ||
| // because the first already covers the padding. | ||
| .mat-mdc-card-subtitle ~ .mat-mdc-card-title, | ||
| .mat-mdc-card-title ~ .mat-mdc-card-subtitle { |
There was a problem hiding this comment.
You could potentially combine it with the declaration above since it's declaring the same styles.
There was a problem hiding this comment.
Done, though it does make the source less pretty since you have to expand the nesting.
| @@ -114,7 +114,7 @@ $mat-typography-level-mappings: ( | |||
| $primary: mat-color(map-get($theme, primary)); | |||
| $accent: mat-color(map-get($theme, accent)); | |||
| $warn: mat-color(map-get($theme, warn)); | |||
| $background: mat-color(map-get($theme, background), background); | |||
| $backgroundPalette: map-get($theme, background); | |||
|
|
||
| @NgModule({ | ||
| imports: [MatCommonModule, CommonModule], | ||
| exports: [MatCard, MatCommonModule], | ||
| declarations: [MatCard], | ||
| exports: CARD_DIRECTIVES, |
There was a problem hiding this comment.
Do we still need to export MatCommonModule?
| // When both a title and a subtitle are used, eliminate the top padding of whichever comes second | ||
| // because the first already covers the padding. | ||
| .mat-mdc-card-subtitle ~ .mat-mdc-card-title, | ||
| .mat-mdc-card-title ~ .mat-mdc-card-subtitle { |
There was a problem hiding this comment.
Done, though it does make the source less pretty since you have to expand the nesting.
crisbeto
added
pr: lgtm
action: merge
(cherry picked from commit f6116ee)
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.