search

adeeshmans / MI-449-js-collect-and-display-information

1 stars 0 forks source link

Project Feedback #1

Open adeeshmans opened 4 years ago

adeeshmans commented 4 years ago

Build a dating profile generator

@egillespie Can you take a look at this? It's hosted here and meets the following criteria:

adeeshmans commented 4 years ago

Hi Erick, I need some help with this project, for some reason, I do not see the preview. Can you point me in the right direction?

egillespie commented 4 years ago

Yeah, I'm happy to help! I think the part preventing your code from working right now is that your HTML file does not have a script element that loads the JavaScript file.

The previous lesson has an example of how to do this or you can check out the HTML from your last JavaScript project. 🙂

I also recommend following the instructions on the first page of this lesson to install and configure the JavaScript linter. It will help avoid common coding issues as you start writing more JavaScript. 😉

adeeshmans commented 4 years ago

lol, I had been changing my code back and forth the whole time 😄, thanks, Erick!!! I tried installing JavaScript linter, but my terminal says command not found

egillespie commented 4 years ago

Hmm, did you run the npm install -g standard command to install the linter?

adeeshmans commented 4 years ago

Ya, I ran the command, "npm: command not found"

egillespie commented 4 years ago

Weird! Did you install Node earlier in the semester? That is the tool used to install surge and I'm surprised we're experiencing this error now...

adeeshmans commented 4 years ago

I am sure that I installed Node earlier this semester because I have surge installed and functioning.

adeeshmans commented 4 years ago

I am googling to find the solution now

egillespie commented 4 years ago

How bizarre. Any luck?

adeeshmans commented 4 years ago

I think the JS standard is already installed in my machine. I think I installed it through the VS code extension marketplace.

egillespie commented 4 years ago

Ah, okay. There are two parts to using the JavaScript linter in VS Code:

  1. Install the standard tool in the terminal with npm install -g standard
  2. Install the StandardJS extension in VS Code

Once both of those are in place and working correctly, restarting VS Code should show red squiggles underneath linter errors in your JavaScript file.

If the StandardJS extension in VS Code can't run the standard tool, you should see an error in the lower-right of VS Code or when you choose View>Output from the file menu.

You can test if the standard tool is installed by running standard scripts.js from your project directory in the terminal.

If any of this appears out of place and you want help troubleshooting, please share a screenshot in a comment. 🙂

adeeshmans commented 4 years ago

when I run ' npm install -g standard ' I get this

Screen Shot 2020-02-15 at 8 20 30 PM Screen Shot 2020-02-15 at 8 18 00 PM

I also resubmitted my code.

egillespie commented 4 years ago

Try running these two commands from your terminal. The first command will install nvm, a useful tool for ensuring a proper version of Node is running on your Mac. The second command installs the latest stable version (12.16.0) of Node.

curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.35.2/install.sh | bash
nvm install v12.16.0

After that, you should be able to run npm install -g standard.

Also, you are welcome to continue working on the project requirements without the linter installed (although it may be a little tedious because the linter reports some bugs as soon as they're typed). When we get the linter working, it will should pretty easy to clean the code up. 👍

adeeshmans commented 4 years ago

Thank you Erick for helping me troubleshoot this problem. Now, the linter is working on my VScode. Also, my code is ready to be checked.

egillespie commented 4 years ago

Woohoo, I'm glad it's working! I can see your hosted site and lots of live updates as I enter my profile info, too. All good things! 🤘

I have a couple of recommendations to better hit the requirements and tighten up your code a little bit. Would you mind trying these out?

Use a function to generate the dating profile

It looks like you have many functions all generating the Preview and Generated HTML — one for each of the inputs. This can introduce a challenge if you need to update the profile because you will have to update the profile in five different places. This is risky because one mistake in any of these functions will cause your code to behave differently depending on which box the user types in.

To avoid this and reduce the amount of code you need to write, you can write one function and give it a name, then use that function for all of your event listeners. Here's some example code that may help:

function updateProfile () {
  var first = firstTextbox.value
  var last = lastTextbox.value
  // ...
  var profile = 'Hi, my name is ' + first ...
  preview.innerHTML = profile
  generated.textContent = profile
}

firstTextbox.addEventListener('input', updateProfile)
lastTextbox.addEventListener('input', updateProfile)

Would you mind swapping out your functions for one reusable function like this?

Use profile format from requirement 1

It looks your code is generating a profile based on some text from the requirements, but I would like the profile string to match the format linked in that first requirement:

<h1>Hi, my name is FIRST_NAME LAST_NAME!</h1>
<p>DESCRIBE_YOURSELF_INFO</p>
<p>
  If you're interested in a date, you can email me at 
  <a href="mailto:EMAIL_ADDRESS" target="_blank">EMAIL_ADDRESS</a>
  or give me a call at
  <a href="tel:PHONE_NUMBER" target="_blank">PHONE_NUMBER</a>.
</p>

Would you mind updating the profile string to match this format, making sure to replace all of the uppercase text with values typed in the text boxes? This will be easier if you create a reusable function first. 😉

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! 🎸

adeeshmans commented 4 years ago

I refactored the code made changed the format

egillespie commented 4 years ago

Sweet, this is coming along really well. This looks so tidy with that function! 😁

There are a few spots that I think can be touched up a bit more. Can you check these out?

Remove unused code

Would you mind removing all of the commented code at the bottom of your JavaScript? It's not used, but the browser still downloads all of it and it could slow the load time of your page. 😉

Save profile string to a variable and reuse it

It looks like you are generating the profile string twice: once for the Preview and once for the Generated HTML.

Would you mind generating the profile string once, assign the result to a variable, and then set the Generated HTML and Preview content using the variable?

This will make it easier to change the profile string — you'll only have to change it once instead of twice — and it guarantees that the Preview and Generated HTML always show the exact same profile.

Remove Hella charming from profile

I see <em>Hella Charming</em>!!! in the generated profile and this isn't part of the dating profile template. Would you mind removing it?

Replace mailto:email and tel:phone with typed values

It looks like the generated profile is always generating links to mailto:email and tel:phone. If I type 555-1234, I would expect the link to be tel:555-1234 so that when I click it, it will start a phone call. Ditto for the email that I type: it should link to mailto:gille143@msu.edu if I type my university email.

Would you mind updating the profile string to replace the hard-coded links with values based on what I type in the email and phone inputs?

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! 🤘

adeeshmans commented 4 years ago

Thank you again for helping me. I have made changes as requested.

egillespie commented 4 years ago

Woohoo, this looks so good, @adeeshmans! I love what you've done with this project. The code is so tidy, contains no duplication, and it works really well. Congrats! 💯 :shipit: