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

0 stars 0 forks source link

Project Feedback #1

Open Storingo opened 7 years ago

Storingo 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 Dylan, your app looks and works great! 👍 But of course, there's always room for improvement! 😆

Meaningful title

I noticed the app is called "This is a title" - not very user friendly! Can you change it to something that will help users recognize the site from a browser tab, share link, or bookmark?

Meaningful, descriptive function & variable names

Your variable names are SOLID. I think your function updateMadLib is named a little misleadingly though. It's kind of a mad lib, but in this context, I'd call it updateProfile or something more specific to this particular app. 😄

Delete commented-out sections

There's some code at the bottom of your js file that it looks like you were just using for reference. As a fellow developer looking at your code, I'm wondering why it's there and what I'm supposed to do with it. Please either add an explanation (in a comment) for why it's there, or delete it! 😄

Break long JavaScript statements into multiple lines

You're off to a good start breaking up your long statements (profileInput.innerHTML = ..., rawProfileInput.innerHTML = ...) into multiple lines. I've been learning about this myself, and it's almost but not quite up to the standard that we're using for the class.

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

Some tips for doing this:

Storingo commented 7 years ago

I believe I have made the necessary changes

KatieMFritz commented 7 years ago

Hey, super awesome commit messages, BTW! 💥

Everything looks perfect except the multi-line indentation. I think I gave you too much information and the main point got lost! Here's only the relevant stuff so you can make that one last change:

If the statement is "something = something else", and you're breaking "something else" into multiple lines, START "something else" on a new line (see the example).

The example from the lesson:

madlibParagraph.innerHTML = 
  'There once were three ' +
  '<strong>' + animal + '</strong> ' +
  'that were very ' +
  '<strong>' + adjective + '</strong> ' +
  'because they couldn\'t stop ' +
  '<strong>' + verb + '</strong> ' +
  'all day.'
Storingo commented 7 years ago

Thank you, and I get it get it know, I'm pretty sure, it should be corrected now.

KatieMFritz commented 7 years ago

Bullseye! 🎯

:shipit:

Seriously, those commit messages. Magnificent.