search

chrisgrizzy / MI-449-SS17-741-js-intro-Vh2K33

0 stars 0 forks source link

Project Feedback #1

Open chrisgrizzy opened 7 years ago

chrisgrizzy 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

Yehoo! 🤾‍♂️ I love the personality here and the style you've added. 😄 There are a few areas for improvement, but nothing major.

Making buttons respond to both mouseenter and click

I see you've created 3 buttons that respond to the mouseenter event and 3 others that respond to click, but we'd like the sound to play whenever either event happens on a button. See if you can update the code to add multiple event listeners per button.

Organizing sound files

When we start to get more files in a project, it's useful to organize them into folders. In this case, I think we have enough sound files that we could create a sounds or audio folder and move them in there. That would clean up the root directory quite a bit!

Mislabeled alt attribute for image

It looks like the value for the alt attribute on the Mario-clapping GIF is bowser. It's been a couple years since I've played a Mario game, but I think this might be mislabeled. 😜

Multiple commands on a single line

While arguments for a function will be separated by a comma, multiple commands inside of a function are actually just separated by new lines and indented inside the function. So for example, this function:

function () {
audio2.play(),console.log('Button 2  was clicked!')
}

Would be better written as:

function () {
  audio2.play()
  console.log('Button 2  was clicked!')
}

And actually, even though console.log is really useful for testing, it's best to delete console.log statements once your program is working. Otherwise, they can quickly take over your code!

Let me know if all that made sense. 😸 As always, if you have any questions, I'm here to help.

chrisgrizzy commented 7 years ago

Updated with changes as requested 😺

chrisvfritz commented 7 years ago

Looking much better!

Human-readable alt attributes

I see you've tried to avoid spaces in your alt attributes with:

This attribute is actually what appears on the page if the image can't load properly, letting the user know what's supposed to be there. That means it not only can have spaces, but actually should, since it's meant to be read by humans. Screen readers for the blind also use this attribute to describe images to their users and similarly, underscores are likely to confuse them a bit.

Better names (with camelCase in JavaScript)

Now that some of the other items are looking better, let's rename audio1, myfunction6, and similar variables to something more descriptive. Trust me, a program can quickly become very difficult to read with names like these! 😹 Even now, in order to find out what myfunction6 does, I had to look at its contents, then search the file for where audio6 was declared, then look at the name of the sound file, which fortunately did have a descriptive name!

To give a few examples, audio1 could be coinSound and myfunction6 could be playYehooSound. This way, it's much easier to tell what each line of the code is doing without having to hunt around.

Also notice that in the examples I just provided, new words are marked with a capital letter. This style of naming things is preferred in JavaScript - see this page of the lesson for more info.

And on this same note, the id attributes have a similar problem, where the names are not very descriptive. Remember though that in HTML, the standard is to separate words with dashes (-) rather than camelCase.

Declare variables before they're used

While the audio variables at the end are made available in previous parts of the file through something JavaScript calls "hoisting", making sense of this behavior can often be a little tricky for newcomers - and there are some instances where it won't happen when declaring a variable.

That's why to avoid confusion, it's best to declare variables before the lines where they're used. Let's do this for the functions as well.

Don't reuse variable names

I actually just noticed that you're re-declaring a buttonElement variable for each button. This works here, but due to the magic "hoisting" behavior that I mentioned earlier, it can also cause difficult-to-predict bugs in many situations. Instead, try giving each button element a unique name.

BONUS HINT 🍪

Notice that you have a bunch of chunks of code that look almost exactly like this?

var audio5 = new Audio('sounds/weehee.wav')
var buttonElement = document.getElementById('button5')
buttonElement.addEventListener('click', myfunction5);
buttonElement.addEventListener('mouseenter', myfunction5);
function myfunction5(){
  audio5.play()
}

The only parts that change are:

As a bonus objective, see if you can create a function that takes two arguments (one for each of the above pieces of information). Then you should be able to reuse that function to dramatically shorten your code. If you can't figure out how to do this last one, don't worry, it's not necessary for me to accept the project and I'll still show you an example of what I meant after I do accept it.

chrisgrizzy commented 7 years ago

Fixes mentioned have been made. I was not able to figure out the bonus, but would love to see how it works 😄

chrisvfritz commented 7 years ago

Excellent! 🎂 🎈😎 :shipit: As promised, here's the shorter version:

var addSoundToButton = function (soundName) {
  var sound = new Audio('sounds/' + soundName + '.wav')
  var element = document.getElementById(soundName + 'Button')
  var playSound = function () {
    sound.play()
  }
  buttonElement.addEventListener('click', playSound)
  buttonElement.addEventListener('mouseenter', playSound)
}

addSoundToButton('coin')
addSoundToButton('powerup')
addSoundToButton('mario')
addSoundToButton('hehoo')
addSoundToButton('weehee')
addSoundToButton('yehoo')

Let me know if you have any questions about it. 🙂