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

0 stars 0 forks source link

Project Feedback #1

Open chrisgrizzy opened 7 years ago

chrisgrizzy 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

Woohoo! ✨ I feel very invested in this project after helping you troubleshoot. Speaking of which, you did a great job staying patient and breaking down the problems until you got everything working. 💪

I was able to use your app to make a dating profile for my cat, but he hasn't gotten any emails yet... 😿

Missing anchor around phone

In the base code for the profile, the phone number is wrapped in an anchor element (link):

<a href="tel:PHONE_NUMBER" target="_blank">PHONE_NUMBER</a>.

It looks a little weird, because it's a special kind of link that lets users initiate a phone call (if they are on their phone, or have phone software on their computer).

That's missing on your preview HTML and raw HTML areas. Can you add it back in? ☎️ 📴


Duplicated string

Since you're assigning the exact same HTML string twice, 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'
element2.innerHTML = 'hello'

Then whenever I want to update 'hello', 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' is a longer, more complex string). If we update string and forget to update the other the same way, then we have an inconsistency. A solution for this would be to move 'hello' to a variable, like this:

var greeting = 'hello'

element1.textContent = greeting
element2.innerHTML = greeting

Now whenever we want to update the 'hello', 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?


Why we use unique, descriptive commit messages

The beauty of git is that by making regular commits, you give yourself lots of "save points." If you mess up later, you can go back to when it was working.

This is a lot easier if each commit has a unique message explaining exactly what changed in that commit. However, for this project, you used basically the same commit message twice. If you needed to go back to a previous commit, you'd have to look at your code to figure out what changed, instead of being able to tell quickly from the commit log. Same if another developer, like me, needed to figure out what had changed.

For this project, I might use commit messages like this:

Do those make sense to you?

Some other tips for commits: :bulb:

You don't have to worry about redoing any of this - but in the future, please give each commit its own specific message describing what you just changed. It will help you be even more of a git wizard, and help you work better with other developers. :muscle:


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:

chrisgrizzy commented 7 years ago

Changes have been updated 😄

KatieMFritz commented 7 years ago

Hi Chris, great job refactoring your javascript to use the profileInfo variable! I also noticed that your latest commit message was really descriptive. 😁

The phone link is almost there, but not quite.

In the base HTML, the link is formatted like this (if my phone number was 517-555-5555)

<a href="tel:517-555-5555" target="_blank">517-555-5555</a>.

In your code, it's like this:

<a href="PHONE_NUMBER:517-555-5555" target="_blank">517-555-5555</a></p>

Take a close look at the href attribute and see if you can spot the difference.

You may have never seen a tel: link before, at least on the code side (you've probably used them on your phone - it's how "tap to call" links work). This article has lots of explanation about the tel: link protocol. Don't worry about the fancy CSS stuff they get into! 😛

Let me know when you've fixed the phone link! 📱

chrisgrizzy commented 7 years ago

Updated phone link :)

KatieMFritz commented 7 years ago

📱 🔥 🚒 *

*Your phone link was so hot it caught on fire and the fire department had to put it out

:shipit: