Add card molecule (Fixes #71)#103
Add card molecule (Fixes #71)#103craigcook merged 2 commits intomozilla:masterfrom alexgibson:card-molecule
Conversation
|
/cc @vincejoyiv |
|
Ok rebased, this should now be ready for review / merge. |
craigcook
left a comment
craigcook
left a comment
There was a problem hiding this comment.
This is so awesome! 😁 Just a few places where I think we can use the bidi mixin, plus one typo.
Not a blocker for this PR but since most of the examples repeat the same markup just with different classes, we could build a single include and pass those classes in. See https://github.com/cloudfour/drizzle/tree/master/src/patterns/components/button for examples.
You can also use the same include/embed method in demos: https://github.com/mozilla/protocol/blob/master/src/pages/demos/article.hbs#L15 But we can merge this now and convert it into variant embeds another time.
|
|
||
| // Preserve aspect ratio to prevent content jumping when media loads. | ||
| // Apply to a direct parent element of an image or video. | ||
| @mixin aspect-ratio($width, $height, $width-wrapper: 100%) { |
| // On small screens cards stack vertically and span full container width. | ||
| // On medium sized screens cards span half width. | ||
| // On large screens cards span quater widths. | ||
| .mzp-l-card-quater { |
|
|
||
| &:nth-child(2), | ||
| &:last-child { | ||
| margin-right: 0; |
There was a problem hiding this comment.
I think you can use the bidi mixin here as well. @include bidi(((margin-right, 0, margin-left, 0),));
If I understand, the mixin behaves a little differently when there are four values instead of three: https://github.com/mozilla/protocol/blob/master/src/assets/sass/protocol/includes/mixins/_bidi.scss#L20
There was a problem hiding this comment.
Ah! i struggled with this and didn't grok the extra value. Thanks for the pointer 👍
There was a problem hiding this comment.
I'm not sure I fully grok it either... I swiped that mixin from MDN. It almost seems to be doing too much. I wonder if it could be simplified.
| } | ||
|
|
||
| &:nth-child(4n) { | ||
| margin-right: 0; |
There was a problem hiding this comment.
Another place where I think the mixin should work with four values: @include bidi(((margin-right, 0, margin-left, 0),));
| } | ||
|
|
||
| &:nth-child(3n) { | ||
| margin-right: 0; |
There was a problem hiding this comment.
@include bidi(((margin-right, 0, margin-left, 0),));
|
@craigcook thanks for the review (and pointers on the bidi mixin!). I added a commit with some fixes. I think using a macro on the card examples is a great suggestion (will add a post-merge task to tidy this up later) |
2 participants
Description
Demo links