JoeViviano / codelab-html-intro

https://github.com
0 stars 0 forks source link

Code Lab Feedback #1

Open JoeViviano opened 8 years ago

JoeViviano commented 8 years ago

@chrisvfritz quack-trucks.surge.sh

chrisvfritz commented 8 years ago

Is the latest version of the code on GitHub? I'm seeing something slightly different from what's hosted at quack-trucks.surge.sh. To commit and push these latest changes you can:

cd the/path/to/codelab-html-intro
git add -A .
git commit -m "description of changes made since last commit"
git push origin master

Let me know if you have any questions! 😃

JoeViviano commented 8 years ago

Ok. I pushed the latest changes. Please let me know if there if still an issue.

chrisvfritz commented 8 years ago

Much better! I see a few more areas for improvement in this code, but then it'll be ship-worthy! 😃

A better alternative to empty p elements

It is technically valid to have p tags without a closing tag, like you have here, but it's generally not recommended, as it can make the code a little bit harder to scan. You have to think a little bit more like a web browser to know where each opening p tag implicitly ends - and it might not always be where you expect.

I'm guessing you used empty p elements in order to create a few empty lines. There's actually another element just for this purpose: <br>. That element is self-closing (just like img or hr) and its sole purpose is to create a line break (like pressing the Enter key in a Word document). Try using that in place of the empty p elements.

Indentation with self-closing tags

There are a few lines where I think some clarification on self-closing tags is in order. For these hr elements below, the headings and content should be on the same line, since they're not technically inside the hrs. Rather, the self-closing tags could be thought of as similar to <hr></hr> - a tag with nothing inside them.

  <h1><strong>Micky J. Mouse</strong></h1>
  <b>PHONE: </b><a href="tel:517.123.4567">517.123.4567</a>, <b>EMAIL:</b> <a href="mailto:MMouse112@aol.com"> MMouse112@aol.com</a>, <b>ADDRESS: </b><a href="https://www.google.com/maps/place/Walmart+Supercenter/@42.7794071,-84.9429703,10z/data=!4m5!1m2!2m1!1sWalmart+Lansing!3m1!1s0x8822f40d7e2f4d9b:0x665e8d2e9f2df6a5">65 Superior Dr, St Johns, MI 48879 </a>
<hr>
  <h3>Education</h3>
  <p>Walt Disney University, BA
<hr>
  <h3>Experience</h3>

In this case, all of these tags are siblings, so I would indent them aligned with each other, like this:

<h1><strong>Micky J. Mouse</strong></h1>
<b>PHONE: </b><a href="tel:517.123.4567">517.123.4567</a>, <b>EMAIL:</b> <a href="mailto:MMouse112@aol.com"> MMouse112@aol.com</a>, <b>ADDRESS: </b><a href="https://www.google.com/maps/place/Walmart+Supercenter/@42.7794071,-84.9429703,10z/data=!4m5!1m2!2m1!1sWalmart+Lansing!3m1!1s0x8822f40d7e2f4d9b:0x665e8d2e9f2df6a5">65 Superior Dr, St Johns, MI 48879 </a>
<hr>
<h3>Education</h3>
<p>Walt Disney University, BA
<hr>
<h3>Experience</h3>

Leaving off the / in self-closing tags

I noticed your image had a closing slash like this:

<img ... />

I recommend leaving out the closing slash, instead writing something like this:

<img ... >

It doesn't change how the tag works, but many companies, such as GitHub and I believe also Google, enforce rules for their code about not using them, since they just introduce more clutter into the code.

JoeViviano commented 8 years ago

Thanks for the comments, Chris. I pushed the revised code to github. Please let me know if I need to make anymore revisions.

Joe

chrisvfritz commented 8 years ago

Looks fantastic! :tada: :shipit: