finchmar / MI-449-SS17-740-js-intro-Vh2K33

0 stars 0 forks source link

Project Feedback #1

Open finchmar opened 7 years ago

finchmar commented 7 years ago

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

egillespie commented 7 years ago

Really good job on this project. It functions great, there's one small request I'd like to make of the JavaScript in your HTML. 🥁

Can you remove the onclick="Sound1()" code in your HTML and give each button an ID so you can do something like this:

document.getElementById('boomButton').addEventListener('click', function () {
  Sound1()
})

Putting all of your JavaScript in a separate file (including adding event handlers) means a designer can worry about the HTML without breaking the code, and a developer can change the code without worrying about accidentally changing the appearance or content of the site.

addEventListener is also more flexible than onclick="..." because you can use addEventListener to add multiple event handlers to an element.


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:

finchmar commented 7 years ago

fix it!

egillespie commented 7 years ago

Awesome, thanks! :shipit: