Skip to content
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

Merged
merged 5 commits into from
May 28, 2018
Merged

Implemented example for <figcaption> tag #941

merged 5 commits into from
May 28, 2018

Conversation

dagolinuxoid
Copy link
Contributor

Take a look. I used few lines of HTML and 22 lines of CSS — a little bit of scrolling won't hurt.

@welcome
Copy link

welcome bot commented May 19, 2018

💖 Thanks for opening this pull request! 💖
Here is a list of things that will help get it across the finish line: - If this is a new or updated CSS interactive example, please ensure that you followed the CSS styleguide - If this is a new or updated JavaScript interactive example, please ensure that you followed the JavaScript styleguide - If your changes affects any of the steps in our contribution docs, please also make the relevant changes there.

@dagolinuxoid dagolinuxoid changed the title Implemented example for <figcapture> tag Implemented example for <figcaption> tag May 19, 2018
@wbamberg wbamberg self-requested a review May 24, 2018 23:51
Copy link
Contributor

@wbamberg wbamberg left a 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;
}
Copy link
Contributor

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">
Copy link
Contributor

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" />

@dagolinuxoid
Copy link
Contributor Author

@wbamberg, thanks for a thorough review. I made some fixes so now it looks decent... I guess :)

Copy link
Contributor

@wbamberg wbamberg left a 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!

@wbamberg wbamberg merged commit c22cf71 into mdn:master May 28, 2018
@welcome
Copy link

welcome bot commented May 28, 2018

Congrats on merging your first pull request! 🎉🎉🎉

wbamberg pushed a commit to wbamberg/interactive-examples that referenced this pull request Jun 8, 2018
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants