search

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

0 stars 0 forks source link

Project Feedback #1

Open davisc91 opened 4 years ago

davisc91 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:

egillespie commented 4 years ago

Wow, great work on this, @davisc91! The code is formatted nicely, the organization of your functions is great, and using two event listeners per input is a pretty nice way to update both previews at once! 👏

Here are some ideas to improve what you've got here. Can you try these out?

Put id attribute on code element

Using an id attribute to lookup and set the textContent of the Generated HTML is totally the correct approach. 👍

I see that you have a p element inside of your code element, though. This isn't valid HTML and may not render correctly in all browsers and may be confusing to anyone using a screen reader.

Instead, would you mind putting the id attribute directly on the code element and removing the nested p element so your page will be fully valid?

Generate the profile once and put it in a variable to reuse it

Using two functions, each as a separate event listener per input element, is a really thoughtful way to update both previews on the page at the same time. I totally dig that! 🤘

There's a slight drawback to this approach, though: the code to generate the profile string is duplicated. This means that if one profile string is updated and the other isn't, the Preview and Generated HTML aren't guaranteed to match.

I have a couple of ideas to resolve this:

  1. Create a third function that gathers the values from the inputs, generates the profile string, and returns it so your two event handlers can both call that one function
  2. Merge your two functions into one function that generates the profile string, saves the result to a variable, then sets the content of the two previews to the value of the variable.

The first solution adds more code, but keeps your slick approach to updating two previews at once. The second solution will reduce the amount of code in your file by almost half, but updates the previews one after the other. Argh, choices!

Would you mind picking one and applying it to your project so the code to generate a profile string is not duplicated?

Generate working links in profile

When I type an email and phone number into your site, the generated links always point to mailto:email and tel:phone:

image

Would you mind updating these to generate links that point at the email and phone number that I type in? In the example above, I would expect the links to point at mailto:gille143@msu.edu and tel:555-1234.

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

davisc91 commented 4 years ago

I think I fixed it, let me know if I did them correctly! For generating the profile once I tried to do the 2nd method.

egillespie commented 4 years ago

Yeah, it's all the code looks great and works as expected! Farewell, double-event-listeners, you'll be missed. I don't have any other recommendations for improvement — amazing work on this project! 💯 :shipit: