-
Notifications
You must be signed in to change notification settings - Fork 5k
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(card): Initial implementation #4630
Conversation
<style> | ||
.demo-card--bg-demo { | ||
height: 21.875rem; /* 350sp */ | ||
background-image: url("images/bamboo.jpg"); |
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.
I hate to have to ask this but do we have IP permissions for these photos?
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.
Also, if we don't want to store images in VC we could always use placekitten 😸
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.
These photos come from unsplash. That license should cover our use-case, right?
I'm happy to replace them with placekitten, but it may make things tricky if we ever decide to use the demos for visual diff testing.
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.
Is there perhaps a version of placekitten where you can select a specific kitten to be used every time? That way, we'd only have to worry about service downtime.
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.
Or we could just use placehold.it for generics.
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.
Tell you what, I think I'll just come up with a non-horrible looking gradient :) Easier than all of this.
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.
Actually, added some of our own imagery instead, kindly provided by @mustafa-x, based on the images on our current microsite. Thanks, @mustafa-x!
(Part of) #4472 |
## Usage | ||
|
||
```html | ||
<div class="mdl-card demo-card"> |
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.
we may want to make an explicit note here that cards don't have default width/height dimensions set (e.g. the demo-card
modifier on here) so that needs to be specified manually.
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.
Good idea. Added some text explaining how cards behave regarding width and height.
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.
nit: instead of having demo-card
as a class (which users might think they have to add) maybe id="example"
instead? Or, just omit the additional class altogether?
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.
omit it completely. Unless we use the class within the same code block to explain something it shouldn't be present.
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.
Bad copy-paste, sorry :)
@sgomes are you going to do horizontal cards in a separate PR? |
5ab8646
to
f61a24c
Compare
@traviskaufman It will be a different PR, yes, although I'm still unsure on whether it will be "horizontal cards" or "vertical cards with a media area to the side", which seems to be a more accurate representation of what's in the spec. Waiting on some info from the Material Design team on that. |
d898284
to
54b9e3c
Compare
I believe all the changes we talked about are in. Some modifications will have to be made once we have @Garbee @traviskaufman: PTAL! |
| `mdl-card__primary` | Defines the primary text / title content block. | | ||
| `mdl-card__title` | A title block. | | ||
| `mdl-card__title--large` | An option for the title, to make it larger. | | ||
| `mdl-card--subtitle` | A subtitle block. | |
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.
mdl-care__subtitle
?
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.
Yup, __
here as it is in the code.
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.
Done.
@sgomes few doc nits, but besides that 💎 LGTM! |
|
||
@import "mdl-typography/mixins"; | ||
|
||
.mdl-card { |
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.
/* postcss-bem-linter: define card */
before the class start please.
4c59eb3
to
ce78ce5
Compare
All the changes should now be in. Want to take another look, @Garbee, or is this good to merge? |
```html | ||
<div class="mdl-card"> | ||
<section class="mdl-card__primary"> | ||
<h1 class="mdl-card--large-title">Title goes here</h1> |
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.
I think the classes need to be updated here
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.
Yes indeed. Fixed!
Just one docs nit, but besides that 💎 LGTM! |
LGTM still. Thank you for adding in the BEM linting comments! |
Implements initial cards, sans expansion and side-placed media.
Implements initial cards, sans expansion and side-placed media.
@Garbee @traviskaufman PTAL!