search

vincentmaule / LAB-101-js-collect-and-display-information

0 stars 0 forks source link

Project Feedback #1

Open vincentmaule opened 3 years ago

vincentmaule commented 3 years ago

Build a dating profile generator

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

egillespie commented 3 years ago

Before I dig in too much, it looks like the Standard linter is reporting 20 errors in the JavaScript file:

image

Would you mind going through the instructions on page 1 of this lesson to install the linter, then use it in VS Code to identify and correct the issues?

You can ignore the warnings if you want (the lines with the yellow squiggles), or replace the var keywords with let.

This will help improve readability of your code, encourage good JavaScript programming habits, and possibly spark some conversations about how certain linter issues can cause problems. 🙂

Comment back when the file's ready to go and I'll take another look. Thanks! 🤖

vincentmaule commented 3 years ago

Able to correct the errors, all that is left is the variable issues which we mentioned we could ignore.

egillespie commented 3 years ago

Sweet, thanks for taking the time to do that. The consistent indentation and formatting help me more reliably notice which lines of code belong to the function and which don't. I'm hoping to also update the JavaScript lessons to use let and const instead of var... those rules weren't a part of Standard JS in April so it's something new to consider. 🙂

Overall the page is functioning really well! I see the preview and generated code updating in real-time for all fields and like the reusable function to accomplish this. Very nice. 👍

I have one recommendation to improve what you've got here that will also help fix a bug in the Preview.

Generate the profile string once and reuse it

It looks like your profile string is being constructed twice: once for the Preview and again for the Generated HTML:

  textINNERHTML.innerHTML =
'<h1>Hi, my name is ' + vvfName + vvlName + '!</h1><p> ' + vvdText + ' </p><p>If you\'re interested in a date, you can email me at <a href="mailto:' + vvuEmail + '" target="_blank">' + vvuEmail + '</a> or give me a call at <a href="tel:' + vuPhone + '" target="_blank">' + vvuPhone + '</a>.</p>'

  textHTML.textContent =
'<h1>Hi, my name is ' + vvfName + vvlName + '!</h1><p> ' + vvdText + ' </p><p>If you\'re interested in a date, you can email me at <a href="mailto:' + vvuEmail + '" target="_blank">' + vvuEmail + '</a> or give me a call at <a href="tel:' + vvuPhone + '" target="_blank">' + vvuPhone + '</a>.</p>'

Doing this work twice means that it's pretty easy for bugs to crop up when changes are made to one of the generated strings and not the other. In fact, this already appears to have happened in the construction of the tel: attribute. The Preview string is using tel:' + vuPhone and the Generated HTML string is using tel:' + vvuPhone.

To avoid these types of bugs, or at least increase their likelihood of being caught, you can generate the string once, assign it to a variable, then reuse that variable like this:

const profile = '<h1>Hi, my name is ' + ...
textINNERHTML.innerHTML = profile
textHTML.textContent = profile

If there are bugs in the generated string, they'll appear in both the Preview and Generated HTML, and they can be fixed by updating one line of code instead of two.

Would you mind trying out this approach and fixing the bug in the telephone link?

Optional: descriptive variable names

Another idea I had that may help prevent bugs like the telephone number link is to use descriptive, whole-word variable names. Without taking in all of the code to fully understand the difference between vuPhone and vvuPhone, it would be pretty easy to accidental add an extra v or leave one out.

For example, using the variable names phoneInput and phoneValue makes it more clear which variable is the element and which contains the text from the element without having to look at the definitions for either variable.

Keep this in mind in the future, it makes for a nice developer experience that your coworkers (and sometimes even yourself) will appreciate. Feel free to try it out on this project too. 😉

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

vincentmaule commented 3 years ago

I was wondering about that one myself, if there was a way to pull from a variable. I overthought it which is a common mistake. Honestly, overthought the variable naming convention as well.

Fixed the issues while provided more documentation upon the variables.

egillespie commented 3 years ago

All your changes look so nice and the results on the hosted site work great! I don't have any other recommendations for improvement here, you're all set! 👏 :shipit: