cbriscoe95 / MI-449-SS17-741-js-collect-and-display-information-kPt9Fl

0 stars 0 forks source link

Project Feedback #1

Open cbriscoe95 opened 7 years ago

cbriscoe95 commented 7 years ago

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

Hey Katie, here's my Dating Profile Generator.

I'm not in love with the implementation that I ended up coming up with. If the user makes a mistake and goes back to change a previous field, the rendered and raw html for the fields after that edited field are no longer displayed. Thoughts on adjusting my implementation to fix that?

Thanks, Chris

KatieMFritz commented 7 years ago

Hi Chris, I was just about to leave you a note about that display bug, and then I saw you beat me to it! :grin: I do indeed have thoughts about it. 😆

I appreciate how you're trying not to display empty placeholder text ("you can email me at or give me a call at "), but you don't need to worry about that for this project. It's okay to have the profile show while it's incomplete. It's more important (at least for this use case) to have it always reflect all the information a user has entered.

To get there, try creating one big function that displays the whole profile (rendered and raw), with event listeners for each input field. Hint: It will be easier if you assign the profile content to a variable.

I'll be around a little bit longer tonight, and most of Sunday and Monday if you want to work on this together over Slack or screenshare! 🚀

PS The email and mail links aren't going to the right locations yet. Take a look at that once you get your event listeners figured out. 😄

cbriscoe95 commented 7 years ago

Hey Katie, check out the implementation I came up with. I would describe it as kinda ugly but it works!

KatieMFritz commented 7 years ago

Getting closer! 😁

Combining duplicate strings with variables

Since you're assigning the exact same HTML string multiple times (the actual profile code, from <h1> to .</p>) , what would you think about assigning it to a variable first, then reusing it when you update the page? So for example, if I'm doing this:

element1.textContent = 'hello' + yourName
element2.innerHTML = 'hello' + yourName

Then whenever I want to update 'hello' + yourName, I have to do it in two places. We're not only too lazy for that 😄, but it can also cause bugs (especially if 'hello' + yourName is a longer, more complex string). If we update one string and forget to update the other the same way, then we have an inconsistency. A solution for this would be to move 'hello' + yourName to a variable, like this:

var greeting = 'hello' + yourName

element1.textContent = greeting
element2.innerHTML = greeting

Now whenever we want to update the greeting, we can do it in just one place. Future development is now much easier and it's more difficult to create bugs. Does that make sense?

Start with creating a variable for your profile HTML, so you only have to write it out once. Then look for other places where you're duplicating code and see if you can use variables to get rid of the duplication! 🚀


Separate event listeners from display function

It's also good to separate your event listeners from your display function(s). That makes it easier to understand code, move things around, and troubleshoot. Right now, your event listeners are mixed into your display function (I realize, looking back at the wording of my last comment, that I might have accidentally pointed you in that direction - I'm sorry!).

To really separate your concerns in this application, you'll want to split things up like this:

  1. Assign variables from the HTML file (this part looks good)
  2. Create a display function that updates the two profile views with the same string of HTML
  3. Listen for the events that should trigger the function

You might want to come back to this part after you've worked on your variables in the first recommendation.

Let me know if you have questions! 😄

cbriscoe95 commented 7 years ago

Hey Katie, I finally worked on a better implementation.

Check it out.

Thanks, Chris

KatieMFritz commented 7 years ago

Wow, this is looking really streamlined and clear! ✨ 💥 ✨ Great job pushing past your block on this. 😁

One tiny formatting thing to improve, and then you'll be all set. 🌴

Indenting multi-line strings in JS

You've done a lovely job concatenating all the pieces of the profile string into one variable (profileText). The convention for a multiple line string like this is to indent everything after the first line by 1 level. It's also common to break the lines up like you would in HTML. So instead of

function profile () {
  var profileText = '<h1>Hi, my name is ' + firstName.value + ' ' + lastName.value + '</h1>' + '<p>' +
  description.value + '</p>' + 'if you\'re interested in a date, you can email me at ' + '<a href="mailto:' +
  email.value + '" target="_blank">' + email.value + '</a>' + ' or give me a call at ' + '<a href="tel:' +
  phone.value + '" target="_blank">' + phone.value + '</a>'

you'll want something like

function profile () {
  var profileText = 
    '<h1>Hi, my name is ' + firstName.value + ' ' + lastName.value + '</h1>' + 
    '<p>' +  description.value + '</p>' + 
    'if you\'re interested in a date, you can email me at ' + 
    ...

This will make it even easier to see that it's all one string, and what it contains.

Make that change, resubmit, and then you'll be good to go! 🚀

cbriscoe95 commented 7 years ago

I fixed the string formatting!

KatieMFritz commented 7 years ago

Yay, you cleared this project! Have an awesome night.

:shipit: