nicoskia / MI-449-SS17-740-js-collect-and-display-information-kPt9Fl

0 stars 0 forks source link

Project Feedback #1

Open nicoskia opened 7 years ago

nicoskia commented 7 years ago

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

KatieMFritz commented 7 years ago

This is dang near perfect! 🌟 Well done! The app works perfectly and your code is really easy to read. Great variable names.

One small improvement:

In your dating.js, where you break the long concatenated strings into multiple lines, each line is currently at the same indentation level:

  previewText.innerHTML =
  '<h1>Hi, my name is ' + firstName + ' ' + lastName + '!</h1>' +
  '<p>' + bio + '</p>' +
  and so on...

This makes it look like each line is a separate statement - like they are here:

  var bio = bioInput.value
  var email = emailInput.value
  var phone = phoneInput.value

When you're breaking one line into multiple lines for space/readability reasons, indent every line after the first one to make it clear that it's a continuation and not a new statement.

After you’ve made your changes and pushed them to GitHub and your hosted site, give it a once-over to make sure it looks right, then comment back here and I’ll take another look.

Thanks! :rocket:

nicoskia commented 7 years ago

Thank you so much for the help, that makes complete sense! I've made the changes and pushed it again.

KatieMFritz commented 7 years ago

You're welcome!

It's close, but now your indentation is starting on the third line of the statement instead of the second.

Here's an example with another long statement:

One line:

var loremIpsum = 'Lorem ipsum dolor sit amet, leo enim odit lectus fermentum, sed et vel, elit sed lobortis id fringilla adipiscing a, pellentesque senectus libero nulla diam tempus praesent, id et maecenas nulla et odio faucibus. Eu curabitur vel tortor ligula, non quisque augue, felis aliquam aliquet rutrum molestie non. Aliquam venenatis proin non, cras sit massa blandit lectus eu rhoncus, nec convallis class quis faucibus, lectus duis mollis a eget molestie, in aliquet dolore consectetuer in. Eget taciti proin, hymenaeos ullamcorper tincidunt eu. '

Broken up:

var loremIpsum = 'Lorem ipsum dolor sit amet, leo enim odit lectus fermentum, 
  sed et vel,  elit sed lobortis id fringilla adipiscing a, pellentesque senectus libero 
  nulla diam tempus praesent, id et maecenas nulla et odio faucibus. 
  Eu curabitur vel tortor ligula, non quisque augue, felis aliquam aliquet rutrum molestie non. 
  Aliquam venenatis proin non, cras sit massa blandit lectus eu rhoncus, nec convallis 
  class quis faucibudit lectus eu rhoncus, nec convallis class quis faucibus, lectus duis 
  mollis a eget molestie, in aliquet dolore consectetuer in. Eget taciti proin, 
  hymenaeos ullamcorper tincidunt eu. '

Does that help? Your code is only different from this because you chose to start the second part of the statement (after =) on its own line - but that's only for presentational reasons, not semantic reasons.

KatieMFritz commented 7 years ago

Oops! My loremIpsum example isn't actually valid, because you can't break up a single string using enter. But you can break it up with concatenation:

var loremIpsum = 
  'Lorem ipsum dolor sit amet, leo enim odit lectus fermentum,' +
  'sed et vel,  elit sed lobortis id fringilla adipiscing a, pellentesque senectus libero' +
  'nulla diam tempus praesent, id et maecenas nulla et odio faucibus.' +
  'Eu curabitur vel tortor ligula, non quisque augue, felis aliquam aliquet rutrum molestie non.'
KatieMFritz commented 7 years ago

And in fact, there's an example of doing this correctly on page 6 of the lesson. Can you tell you're my first review for this lesson? 😬

Thanks for your patience!

nicoskia commented 7 years ago

Got it, thank you! No worries, I understand what you're getting at. I forgot that I had started the second part on its own line. It should be fixed now!

KatieMFritz commented 7 years ago

Awesome! 🚀 :shipit: