-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(image-list): Add base styles and mixins for Standard Image List #2367
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2367 +/- ##
=======================================
Coverage 98.85% 98.85%
=======================================
Files 100 100
Lines 4118 4118
Branches 527 527
=======================================
Hits 4071 4071
Misses 47 47 Continue to review full report at Codecov.
|
917f336
to
d13bc6a
Compare
805a1e7
to
2c4ad16
Compare
packages/mdc-image-list/README.md
Outdated
--- | --- | ||
`mdc-image-list` | Mandatory. Indicates the root Image List element. | ||
`mdc-image-list__item` | Mandatory. Indicates each item in an Image List. | ||
`mdc-image-list__image-aspect-container` | Optional. Parent of each item's image element, responsible for constraining aspect ratio. This element may be omitted entirely if images are alreadysized to the correct aspect ratio. |
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 space after "already"
.mdc-image-list { | ||
display: flex; | ||
flex-wrap: wrap; | ||
margin: 0 auto; // UA override |
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.
What's UA override?
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.
User agent override. As in, this is overriding user agent styles. I can make this comment clearer, it was carried over from my prototype.
} | ||
|
||
@at-root { | ||
@include mdc-image-list-aspect(1); |
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.
Why is this under @at-root
instead of under .mdc-image-list
?
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.
Because it already creates a 1-class selector and there's no reason to make it a 2-class selector. Similar to instances where we moved mixin invocations in other packages' base styles to @at-root
to decrease specificity and take advantage of BEM.
packages/mdc-image-list/README.md
Outdated
`mdc-image-list__image-aspect-container` | Optional. Parent of each item's image element, responsible for constraining aspect ratio. This element may be omitted entirely if images are alreadysized to the correct aspect ratio. | ||
`mdc-image-list__image` | Mandatory. Indicates the image element in each item. | ||
`mdc-image-list__supporting` | Optional. Indicates the area within each item containing the supporting text label, if the Image List contains text labels. | ||
`mdc-image-list__label` | Optional. Indicates the text label in each item, if the Image List contains text labels. |
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.
Add row for mdc-image-list--protected
.
Also I think we should either rename it to mdc-image-list--protected-text
, or rename it to mdc-image-list__supporting--protected
and apply it on the mdc-image-list__supporting
element.
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.
Haha, whoops, yeah. I ended up realizing protected
was missing and adding it before I saw this comment, which probably came in around the same time.
width: 100%; | ||
height: 48px; | ||
padding: 0 16px; | ||
background: rgba(0, 0, 0, .6); |
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.
Should we make this a constant if it's the baseline default?
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.
Yeah, I realized a couple of things could probably be constants, so I'll get on that.
Resolves #2314.
Work on actionable images and iconography will be done separately.