I need help cleaning up this code. Adding aspect ratio to it

[PaulOB]

@asasass

What did I do wrong

You didn’t place the ratio=keeper class on the video-frame element as I told you to do. Instead you put it on the div that is being replaced with an iframe?

You also didn’t absolutely place the iframe.

Here is the code again with the useless elements removed.

[asasass]

Why did you include an empty class="" ?

[PaulOB]

@asasass

Why did you include an empty class="" ?

Just to show you where I moved it from

[asasass]

1) Adding this breaks the code, how is that fixed?

margin: auto 0 50px; https://jsfiddle.net/fg5v29qp/1/

So I wrote it this way: https://jsfiddle.net/c36m1tz2/

.container {
  max-width: 642px;
  margin: auto;
  margin-top: 50px;
}

2) 2nd question: https://jsfiddle.net/82fbg931/1/

I am trying to do this:

This was my attempt: https://jsfiddle.net/b85h9zfr/

.emptyOne {
  margin-top: 15px;
}

.emptyTwo {
  padding-top: 46px;
  background: #121212;
}

.footer {
  border: 1px solid #0059dd;
  padding-top: 60px;
  background: #121212;
}

 <div class="emptyOne">
  </div>
</div>

<div class="emptyTwo">
</div>

<div class="footer">
</div>

3) Did I do this the right way?

1st one gets 15px margin top,

Only the last one gets 30px margin bottom.

.ratio-keeper:first-child {
  margin: 15px 0 0;
}
.ratio-keeper:not(:first-child) {
  margin: 30px 0 0;
}

[PaulOB]

@asasass

.container {
  max-width: 642px;
  margin: auto;
  margin-top: 50px;
}

The shorthand is margin:0 auto 50px. (Probably lesson one in css).

@asasass

.emptyOne {
  margin-top: 15px;
}

.emptyTwo {
  padding-top: 46px;
  background: #121212;
}

No, I’ve told you before not to use empty elements to make space. Get rid of empty one and empty 2.

For the empty-one div you just need a margin-bottom of 30px on the last-child similar to what you did with the first child…

.ratio-keeper:last-child {
  margin: 0 0 15px;
}

Then the footer can have its own margin-top. instead of empty2.

.footer {
  margin-top:46px;
  border: 1px solid #0059dd;
  padding-top: 60px;
  background: #121212;
}

[asasass]

margin:0 auto 50px

Doesn’t give me this look: https://jsfiddle.net/vt15d09h/

Using this does:

  margin: auto;
  margin-top: 50px;

https://jsfiddle.net/b85h9zfr/

[PaulOB]

@asasass

margin:0 auto 50px

I did that as a trick to get you to think for yourself.

It should be:

margin: 50px auto 0;

Margin shorthand rules for one, two, three and four value declarations are:

Shorthand properties - CSS: Cascading Style Sheets | MDN

* When **one** value is specified, it applies the same margin to **all four sides**.
* When **two** values are specified, the first margin applies to the **top and bottom**, the second to the **left and right**.
* When **three** values are specified, the first margin applies to the **top**, the second to the **left and right**, the third to the **bottom**.
* When **four** values are specified, the margins apply to the **top**, **right**, **bottom**, and **left** in that order (clockwise).

[m_hutley]

wouldnt… that technically make the equivalency 50px auto auto? Cause a one value would be top&left&right&bottom?

[asasass]

I am trying to replicate this:

How do I do that?

https://jsfiddle.net/4hbvz29m/

[PaulOB]

The easiest way is to wrap the container around each ratio keeper and give some top and bottom padding and then a background color… You then remove the margins from the ratio keeper and do it on the container instead.

Here’s the updated code although I haven’t checked everything but you should get the idea.