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

0 stars 0 forks source link

Project Feedback #1

Open austin1244 opened 7 years ago

austin1244 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

Nice! ⭐️ I can see you know your way around JavaScript objects. 😄

Since you already have some background here, I know it's tempting to just skip the lesson content, but I recommend going back and reading it. We work hard to ensure the content is 100% up-to-date and we want to make sure you're building off a solid foundation as we dive into more complex topics. When you go back, you'll find more information on some of these issues:

JS Standard Linter

The lesson walks through installation of a linter that we'll be using to standardize our coding style across this and all future JavaScript projects. It will also automatically catch many errors/typos, before even saving, so it can be a great boon to productivity. 🙂

camelCase in variable names

For variable names such as firstname, the best practice in JavaScript is actually to use camelCase rather than all lowercase. See the first JS lesson for more details.

We'd also like to see full words in variable names, rather than abbreviations like descript. It just makes things a little clearer. 🎈

input vs keyup events

I actually recommend the input event over keyup. The keyup event only fires when a user depresses a key, which is often not a great user experience, since usually things happen as soon as a user presses the key down. Even beyond that though, keyup is limited. For example, if you right click on input/textarea, then click Paste, no event is fired (but it does fire unnecessarily when pressing arrow keys and others that have no effect on the content).

The input element has none of these drawbacks and is well-supported in all modern browsers.

Obsolete xmp element

As you'll see here, the xmp element is actually obsolete, so should not be used in modern projects. You'll find an alternative in the lesson.

Unnecessary useCapture argument for addEventListener

Since the useCapture argument (i.e. the 3rd argument) for addEventListener actually defaults to false, specifying it isn't necessary here. It can be removed.

Missing quotes on href elements

While quotes are indeed optional when an attribute's value is guaranteed to have no whitespace, that's not actually a guarantee we can make with either the email address or phone number, since we're filling those in from user input.

In the case of the email, a non-savvy user could write it as name @ gmail.com with spaces around the @. While technically invalid, some email clients will actually compensate for this, stripping the spaces - or at least warn about this behavior when actually sending an email. Without quotes however, users visiting that profile and clicking the link would not see the address in their email client, as it would be cut off at the first space.

As for the phone number, it's definitely not uncommon for people to include spaces (e.g. (517) 555-5555), so it's better to include quotes around the value there as well.

getElementsByTagName vs querySelectorAll

I really like that you're selecting these elements all together and iterating over them! Here's a trick though. You could actually combine your two lists into one with querySelectorAll. This allows you to query items by CSS selectors.

For example, since all your form elements have a data-name attribute, you could select them all together with:

document.querySelectorAll('[data-name]')

Please don't hesitate to let me know if you have any questions, whether it's about anything here or just about JavaScript / web development in general. 😊 You can also reach me on Slack by @chrisvfritz if you prefer real-time. I'm looking forward to your next iteration! 🚀

austin1244 commented 7 years ago

@chrisvfritz I believe i fixed the issue now if you want to take anther look

I do look over the lessons, but when I feel like i know the code segment or I am familiar with the content, I skip it. The problem with that is that the lessons are more dense than I anticipated, so I will try to read carefully on the lessons from now on.

chrisvfritz commented 7 years ago

Sounds good! This looks fantastic and I really look forward to your future projects. 🎉 :shipit: Since you're more advanced than most students, also please don't hesitate to let us know if you think of an extra challenge that could make a project more interesting for you (and more likely to stretch your skills). 👍