kellyskarritt / MI-449-SS17-740-js-collect-and-display-information-kPt9Fl

0 stars 1 forks source link

Project Feedback #1

Open kellyskarritt opened 7 years ago

kellyskarritt commented 7 years ago

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

chrisvfritz commented 7 years ago

Very nice! 🎉 I can tell you're a professional. 💼 😄 I just copied the generated HTML into a new site I sent to Katie. Wish me luck! 😜

A few areas for improvement:

More readable multi-line strings

Your JavaScript looks really beautiful, apart from the multi-line strings. I know, they can really be a pain to make look good, but there are a couple things that can help:

To give you an idea of what I mean, you could rewrite this:

preview.innerHTML =
'<h1>Hi, my name is ' + firstName + ' ' + lastName +
'!</h1> <p>' + describe + '</p> <p>If you are interested in a date, you can email me at <a href="mailto:' +
email + '" target="_blank">' + email + '</a> or give me a call at <a href="tel:' + phone +
 '" target="_blank">' + phone + '</a>.</p>'

as:

preview.innerHTML =
  '<h1>Hi, my name is ' + firstName + ' ' + lastName + '!</h1>' +
  '<p>' + describe + '</p>' +
  '<p>' +
    'If you are interested in a date, you can email me at ' +
    '<a href="mailto:' + email + '" target="_blank">' + email + '</a> ' +
    'or give me a call at ' +
    '<a href="tel:' + phone + '" target="_blank">' + phone + '</a>.' +
  '</p>'

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? This makes it so that if you wanted to update the HTML in the future, you'd only have one place to do it.


I've really been enjoying the quality of your projects and I'm thinking of pushing you a little further before the squirrel this time. Before I do that though, can you tell me more about your current experience with JavaScript? Are there any issues/questions with inputs or writing to the page that you've encountered in your job? Or something else related to this project that you might be interested in learning more about?

kellyskarritt commented 7 years ago

Thank you!! I fixed the string so it is more readable. However, I am a little confused on what you mean by the duplicated string. I get the idea but I'm not sure how to execute it. At my job I normally use HTML and TWIG, along with CSS and LESS. I haven't encountered any use of javascript there yet, so it's still pretty new to me. I have used it a few times in the past, but not often.

chrisvfritz commented 7 years ago

I fixed the string so it is more readable.

Looking much better! There's just one tiny thing:

preview.innerHTML =
'<h1>Hi, my name is ' + firstName + ' ' + lastName + '!</h1>' +
'<p>' + describe + '</p>' +
'<p>' +
  'If you are interested in a date, you can email me at ' +
  '<a href="mailto:' + email + '" target="_blank">' + email + '</a> ' +
  'or give me a call at ' +
  '<a href="tel:' + phone + '" target="_blank">' + phone + '</a>.' +
'</p>'

When quickly scanning your code after not looking at it for a while, you'd have to look closely to notice that the lines with the strings are actually a continuation of the preview.innerHTML = statement. To make this more clear, you can indent these strings like so:

preview.innerHTML =
  '<h1>Hi, my name is ' + firstName + ' ' + lastName + '!</h1>' +
  '<p>' + describe + '</p>' +
  '<p>' +
    'If you are interested in a date, you can email me at ' +
    '<a href="mailto:' + email + '" target="_blank">' + email + '</a> ' +
    'or give me a call at ' +
    '<a href="tel:' + phone + '" target="_blank">' + phone + '</a>.' +
  '</p>'

I am a little confused on what you mean by the duplicated string. I get the idea but I'm not sure how to execute it.

Great question! 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?

At my job I normally use HTML and TWIG, along with CSS and LESS. I haven't encountered any use of javascript there yet, so it's still pretty new to me. I have used it a few times in the past, but not often.

Wow, I really couldn't tell you're relatively new to JavaScript! You must be reading the lessons very carefully to be producing work of this quality so soon. 👏 I think I have a good extra objective for you:

A very common feature is copying text to the clipboard when a user clicks on something. See if you can figure out how to use clipboard.js to make it so that when you click on the pre element, the generated HTML is copied to the clipboard. Ignore any of their instructions that mention installing with NPM - that's for more complex projects. Even though you might see some things you're unfamiliar with, try reading the documentation behind the link to learn how to make this happen.

And if you feel like it, two stretch goals:

Of course, if you have any questions or want a little more guidance, please don't hesitate to let me know. As I'm sure you've learned at your job, you're allowed and encouraged to ask questions when there's something you could use some help with! 😄

kellyskarritt commented 7 years ago

I fixed the issues with the duplicated string! As for the rest, I figured out how to change the cursor. However, I am confused on how to implement the clipboard code into the pre tags. Most of the examples I saw were buttons so I am not sure how to get it working without a button.

chrisvfritz commented 7 years ago

No worries. 🙂 So here's a secret - there's actually nothing special about a button element, except how it looks. That means whatever works on a button element would also work on a p, img, h1, or even the html element (though I wouldn't recommend it 😅)! For the examples where there are two elements - a button and something else - there's actually no reason you can't apply the same code to a single element.

Let me know if that helps. Otherwise, I'm happy to come back to provide more info. Also feel free to reach out on Slack for a real-time conversation if that would be easier. I'm a night owl. 🌙 😄🌙

chrisvfritz commented 7 years ago

Just wanted to let you know, I just left you some more information on this project in Slack. 🙂

chrisvfritz commented 7 years ago

:shipit: 🎉