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

0 stars 0 forks source link

Project Feedback #1

Open jmartinez95 opened 7 years ago

jmartinez95 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

Hi Jake, your app looks fantastic and works just as intended! 🌟

Here are two ways you can improve your code:

Break long JavaScript statements into multiple lines

You've done a good job breaking your long JavaScript statements (for the preview HTML and raw HTML) into multiple lines so it's easy to read. 😄

previewlib.innerHTML = 
'<h1>Hi, my name is ' + fname + ' ' + lname + '!</h1> ' +
'<p>' + bio + '</p> ' +
...

However, the indentation is a little off in those blocks. Every line after the first should be indented exactly one level to show that it's all part of the same statement (I think it's okay to have a little nesting beyond that, as you have done).

Check out Spreading statements over mulitple lines on page 6 of the "Use JS to collect, remember, and display info" lesson for an example.


Descriptive JS variable names in camelCase

You've done a pretty nice job with your variable names - I can tell what they are describing. However, you're not using camelCase consistently in your multi-word variables, like previewlib. Please update your variable names so if they are more than one word, the second and all subsequent words are capitalized. Check out the camelCase note in the "Use JavaScript to build more interactive webpages" lesson if you need a refresher!


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:

jmartinez95 commented 7 years ago

@KatieMFritz Fixed.

KatieMFritz commented 7 years ago

This. ✨ Looks. 🔥 Fantastic. 🎉

:shipit: