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

0 stars 0 forks source link

Project Feedback #1

Open OneRandomMikey opened 7 years ago

OneRandomMikey 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

Zehua, fantastic start! :fire:

Your app works great, your variable names are clear, and your code is pretty concise. ✨

Since you're assigning the exact same HTML string twice (the actual profile code, from <h1> to .</p>) , what would you think about assigning it to a variable first, then reusing it when you update the page? So for example, if I'm doing this:

element1.textContent = 'hello' + yourName
element2.innerHTML = 'hello' + yourName

Then whenever I want to update 'hello' + yourName, I have to do it in two places. We're not only too lazy for that 😄, but it can also cause bugs (especially if 'hello' + yourName is a longer, more complex string). If we update one string and forget to update the other the same way, then we have an inconsistency. A solution for this would be to move 'hello' + yourName to a variable, like this:

var greeting = 'hello' + yourName

element1.textContent = greeting
element2.innerHTML = greeting

Now whenever we want to update the greeting, we can do it in just one place. Future development is now much easier and it's more difficult to create bugs. Does that make sense?

OneRandomMikey commented 7 years ago

I made some changes.

KatieMFritz commented 7 years ago

Yes, you're on the right track! :fire:

Now you can clean it up.

var element = 
  '<h1>Hi, my name is ' + firstName + ' ' + lastName + '!</h1>' +
  '<p>' + describe + '</p>' +
  '<p>' +
  // and so on...
OneRandomMikey commented 7 years ago

I made changes.

KatieMFritz commented 7 years ago
more awesome than a double rainbow

:shipit: