-
Notifications
You must be signed in to change notification settings - Fork 511
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
Implemented example for <figcaption> tag #941
Conversation
💖 Thanks for opening this pull request! 💖 |
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.
Thanks for your contribution, and sorry to be so slow reviewing this. In general this looks great!
I just have a couple of very nitpicky comments.
One other comment is that we're always attributing the images we use. In this case we'd use something like:
Photo by Ricky Kharawala on Unsplash
Perhaps we could include it in the figcaption
itself, by making it something like:
Hamster by Ricky Kharawala on Unsplash
...if you do that, it would probably be worth setting margin: 1rem;
or something like that on the figure
, so the caption gets a bit more horizontal space and won't wrap so readily.
background: #222; | ||
color: #fff; | ||
padding: 3px 0; | ||
} |
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.
Files should end with a newline, here and in the other cases.
@@ -0,0 +1,4 @@ | |||
<figure> | |||
<img src="/media/examples/hamster.jpg" alt="hamster"> |
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.
In these examples we prefer to close tags even when it's not needed:
<img src="/media/examples/hamster.jpg" alt="hamster" />
@wbamberg, thanks for a thorough review. I made some fixes so now it looks decent... I guess :) |
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.
Looks good to me @dagolinuxoid ! Thanks for your contribution!
Congrats on merging your first pull request! 🎉🎉🎉 |
* upstream/master: adding oblique plus angle option to font-style example (mdn#963) fix(tabbed-editor): Apply output class to output container (mdn#961) Add rt example. (mdn#957) chore(deps): update dependency prettier to v1.13.4 (mdn#953) chore(deps): update dependency all-contributors-cli to v4.11.2 (mdn#954) chore(deps): update dependency jest to v23.1.0 (mdn#955) Fix console util to support negative zero (mdn#960) Add rp example. (mdn#945) Bug 1462897 - Directly use Math.round in the demo code. (mdn#956) chore(docs): update README with maintainers and good first issues (mdn#934) Implemented example for <figcaption> tag (mdn#941) Font variation settings (mdn#948) Fix font optical sizing (mdn#947) chore(deps): update dependency jest to v23 (mdn#944) chore(deps): update dependency prettier to v1.13.0 (mdn#950) chore(community): add @elharony as contributor (mdn#951) Add object-keys.html (mdn#937)
Take a look. I used few lines of HTML and 22 lines of CSS — a little bit of scrolling won't hurt.