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

0 stars 0 forks source link

Project Feedback #1

Open friendo9876 opened 7 years ago

friendo9876 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 Mateen! I checked your JS before I started reviewing, and I see you used jQuery for this project too. Check out my comment on this issue for a brief explanation of why we want you to use plain JavaScript for these projects instead of jQuery.

Please refactor your JS to not use jQuery and leave a comment here when it's updated. Thank you!

friendo9876 commented 7 years ago

I have updated my code. Am I not allowed to use Jquery at all in this course?

KatieMFritz commented 7 years ago

Thanks for refactoring! The front end still works great. 😄 Here are some ways to improve your code.

To answer your question: No, we probably won't be using jQuery in this course, though we might use other libraries. We want to make sure everyone gets a solid understanding of how JavaScript works. That said, if there's something you want to do to go above and beyond in a project, and it can only be done with jQuery or another library, that's okay! 🍕

Remove link to jQuery

Looks like you forgot to take out the script adding jQuery to your project (line 60 of index.html). Please delete that, so we can be sure that your script works fully without jQuery! 👍


Input vs Keyup

We 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. 😁


Reduce repeated code

I also see some opportunities to make your JavaScript more concise! 🦑

First, for each of the input fields, you're defining a variable:

var FnameInput = document.getElementById('Fname')

You're using it to create an event listener later. But you're getting the exact same elements inside your updateText function:

var Fname = document.getElementById('Fname').value

Instead, you can reuse those variables you've already created:

var Fname = FnameInput.value

See? 👀


Use descriptive variable names

Most of your variable names are nice and descriptive, but there are three that are kind of vague: info, interested, and interested2. What are these variables really describing? Consider bio or description instead of info, and contact or contactInfo instead of interested. 🙂

It looks like interested2 is just named that because it's the second line of interested. I recommend concatenating the two lines with +. You can still put it on multiple lines. Check out Spreading statements over mulitple lines on page 6 of this lesson for an example.

:bulb: Some tips for spreading JS statements over multiple lines:

  • Choose your break points. Look for natural breaks in the statement, like commas, elements tags, or concatenation breaks. The lines don't have to be even.
  • If your statement is a chunk of HTML, format it like you would format it as HTML.
  • You can't just use enter to break up a string, but you can separate it with concatenation (part 1 + part 2)
  • Be careful with indentation. Every line after the first should be indented exactly one level to show that it's all part of the same statement.
  • 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).

Use camelCase for JS variable names

I notice you're not using camelCase consistently in your multi-word variables, like Fname and Lname. Variables like firstNameInput are 🔥 . 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 js-intro 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:

friendo9876 commented 7 years ago

I have updated by code, please look again

KatieMFritz commented 7 years ago

Super close now! 🌟

Your variable names are way better. 😁 The only one that is still unclear to me is All. I mean, I can figure it out from context, but something like allText or fullProfile would be clearer.

Take another look at the camelCase docs and update the casing of your variables. Let me know if you have any questions - I'm happy to help! 🐫

friendo9876 commented 7 years ago

I changed the variable name so please look at one more time

friendo9876 commented 7 years ago

Also now I think its actually Camel Case so please check again

KatieMFritz commented 7 years ago
it's so beautiful

Great job iterating! :shipit: 🎆 💥