-
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(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.