suuuth / MI-449-SS18-740-js-collect-and-display-information-kPt9Fl

0 stars 0 forks source link

Project Feedback #1

Open suuuth opened 6 years ago

suuuth commented 6 years ago

Build a dating profile generator

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

KatieMFritz commented 6 years ago

Hey Kevin, great start to this! 👍

Some ways to improve:

1. Fix linting errors

I'm seeing 3 JS Standard linter errors for js-collect-info.js. Could you please fix those? 🐉 Be sure to check again after your other changes!

2. Use textcontent instead of innerHTML to display raw code

You won't have to escape your HTML tags if you use the textContent method instead of innerHTML to display the raw code. 💡 This also means you'll be able to do the next thing:

3. reduce duplicate strings with variables

Since you're assigning the exact same HTML string twice (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? ✈️

4. Use a more specific function name

update isn't inaccurate for your function, but I'd like to see something a little more specific. Update what? 😄

5. Optional task: Add navigation to your page

As you add more things to this page, it's going to take more scrolling to get to specific sections. Would you mind adding an in-page navigation/table of contents to the top, that jumps to specific projects when you click on the name of one? Hint: you'll want to use anchor links. 🔖


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:

suuuth commented 6 years ago

just pushed an update @KatieMFritz

KatieMFritz commented 6 years ago

Looks wonderful all around! 🌟 🎉 🍕 I'm still seeing one JS-Standard linter error on my end though. Are you seeing it?

suuuth commented 6 years ago

should be fixed now @KatieMFritz !

KatieMFritz commented 6 years ago

cat-fist-bump-it

:shipit: