manotejmeka / MI-449-SS17-741-js-collect-and-display-information-kPt9Fl

0 stars 0 forks source link

Project Feedback #1

Open manotejmeka opened 7 years ago

manotejmeka commented 7 years ago

@chrisvfritz Can you take a look at this? It's hosted here and meets the following criteria:

chrisvfritz commented 7 years ago

This is looking good! We're most of the way there. 😄 There are just a few areas for improvement:

Formatting multiline strings

For multiline strings of HTML, I actually recommend formatting them like you would actual HTML, with the same consistent indentation. For example:

var myString =
  '<h3>' + todoListTitle + '</h3>'
  '<ul>' +
    '<li>' + todoListOne + '</li>' +
    '<li>' + todoListTwo + '</li>' +
    '<li>' + todoListThree + '</li>' +
  '</ul>'

This makes it quite a bit easier to read if you have to modify it later.

Issues in generated HTML

It looks like your generated HTML isn't quite the same as the sample. To make the issues easier to see, here's what your HTML looks like with the example data typed into every field:

<h1>Hi, my name is FIRST_NAME LAST_NAME !</h1>
<p>DESCRIBE_YOURSELF_INFO</p>
<p> 
  If you're interested in a date, you can email me at 
  <a href='mailto:'EMAIL_ADDRESS' target='_blank'>EMAIL_ADDRESS</a> 
  or give me a call at 
  <a href='tel:PHONE_NUMBER' target='_blank'>PHONE_NUMBER</a>.
</p>

And this is what it's supposed to look like:

<h1>Hi, my name is FIRST_NAME LAST_NAME!</h1>
<p>DESCRIBE_YOURSELF_INFO</p>
<p>
  If you're interested in a date, you can email me at 
  <a href="mailto:EMAIL_ADDRESS" target="_blank">EMAIL_ADDRESS</a>
  or give me a call at
  <a href="tel:PHONE_NUMBER" target="_blank">PHONE_NUMBER</a>.
</p>

Using functions by their reference

Whenever you have a function that just calls another function inside it, like this:

function () {
  addText()
}

It can actually be shortened to just:

addText

Without the (), the function actually changes from a function call to a function reference. That means instead of an order (e.g. "Do this!"), it's an instruction that can be passed to other functions, like addEventListener.

JS Standard code style

It looks like this JavaScript doesn't quite meet the JS Standard code style. Do you have it working in Atom? If the linting errors don't seem to be showing up for you, just let me know and I'll be happy to help you troubleshoot. 🙂

manotejmeka commented 7 years ago

@chrisvfritz Made the Styling changes you have asked for

chrisvfritz commented 7 years ago

The code style looks much better! Let me know when the other items are also taken care of and of course, if you have any questions about them. 🙂

manotejmeka commented 7 years ago

@chrisvfritz All done with changes

manotejmeka commented 7 years ago

@chrisvfritz Fixed the addListener calls.

chrisvfritz commented 7 years ago

Excellent! Nice iterations here. 👍 🔥 💥 :shipit: