5
\$\begingroup\$

This is the first jQuery program I've written, and it doesn't do much, but I'd appreciate the feedback. Just click on the different colored buttons to do things.

Codepen

var $greenElement = $(
  '<div style="width:750px;height:25px; \
  background-color:green;" class="green-bar"> \
  <p>Hello</p> \
	</div>'
);

var $redElement = $(
  '<div style="width:750px;height:25px; \
  background-color:red;" class="red-bar"> \
  <p>Goodbye</p> \
  </div>'
);

/* Main JQuery loop */
$(document).ready(function() {
  
  /* Add a red item and fade to the right value*/
  $('.add-item-red').mouseenter(function() {
    $(this).fadeTo('fast', 0.4);
    $(this).click(function() {
      $('.output-start').after($redElement);
    });
  });
  
  /* Check if the mouse is no longer on red button */
  $('.add-item-red').mouseout(function() {
    $(this).fadeTo('fast', 1);
  });
  
  /* Add a green item and fade to the right amount */
  $('.add-item-green').mouseenter(function() {
    $(this).fadeTo('fast', 0.4);
    $(this).click(function() {
      $('.output-start').after($greenElement);
    });
  });
  
  /* Check if mouse is no longer on green button */
  $('.add-item-green').mouseout(function() {
		$(this).fadeTo('fast', 1);
  });
  
  /* Reset all the items */
  $('.reset-items').mouseenter(function() {
    $(this).fadeTo('fast', 0.4);
    $('.reset-items').click(function() {
      $('.green-bar').remove();
    	$('.red-bar').remove();
    });
  });
  
  /* Check if mouse is no longer on white button */
  $('.reset-items').mouseout(function() {
    $(this).fadeTo('fast', 1);
  });
});
body {
  background-color: black;
}

.button-bar div{
  display: table;
  display: table-cell;
}

.add-item-red {
  width: 50px;
  height: 50px;
  background-color: red;
  border-color: black;
  border-style: solid;
  border-width: 3.5px;
}

.add-item-green {
  width: 50px;
  height: 50px;
  background-color: green;
  border-color: black;
  border-style: solid;
  border-width: 3.5px;
}

.reset-items {
  width: 50px;
  height: 50px;
  background-color: white;
  border-color: black;
  border-style: solid;
  border-width: 3.5px;
}

.output-start {
  width: 50px;
  height: 15px;
}
<html>
  <head>
    <script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
  </head>
  <body>
    <div class="button-bar">
      <div class="add-item-red">
      </div>
      <div class="add-item-green">
      </div>
      <div class="reset-items">
      </div>
    </div>
    <div class="output-start">
    </div>
  </body>
</html>

\$\endgroup\$
  • 3
    \$\begingroup\$ Note the codepen isn't necessary as we have code snippets here on CodeReview, because we are just swanky like that. \$\endgroup\$ – Dan Pantry Mar 15 '15 at 0:59
  • \$\begingroup\$ @DanPantry I know I just figured that some people might want an external link that they could edit. \$\endgroup\$ – Ethan Bierlein Mar 15 '15 at 2:33
7
\$\begingroup\$

Ok, so there's two different things here:

  1. Hover effects (fade in/out)
  2. Actions

Now, #1 is the same for all three elements, and thus shouldn't need to be repeated in code. But #2 is expressly different for each button, which means it probably shouldn't mixed with the hover effects.

Aside: Speaking of duplication, your CSS is full of it too. You should look into that. You should also avoid naming things for how they look. E.g. add-item-red is fine until that item is no longer red; usually it'll have some purpose other than being red. Its color is a presentation detail. Name stuff for what it does, not how it looks.

Anyway, these days (read: modern browsers), I'd probably just use CSS transitions to handle the hover effect; no javascript necessary. For instance:

div {
  background: green;
  padding: 0.3em;
  display: inline-block;
  transition-duration: 0.5s;
}

div:hover {
  opacity: 0.4;
}
<div>Hover</div>

But since this is about jQuery, here's how I'd handle it with a common class name for all the buttons (which, incidentally, I'd use <button> tags for), and jQuery's appropriately named .hover() event handler.

$(".fade").hover(function () {
  $(this).fadeTo('fast', 0.4);
}, function () {
  $(this).fadeTo('fast', 1.0);
});
body { background: black; }

button {
  border: none;
  padding: 1em;
}

#helloButton   { background: green; }
#goodbyeButton { background: red; }
#clearButton   { background: white; }
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

<button type="button" id="helloButton" class="fade">One</button>
<button type="button" id="goodbyeButton" class="fade">Two</button>
<button type="button" id="clearButton" class="fade">Three</button>

Now, you're using mouseenter combined with mouseout. But that's not really "symmetrical". The opposite of mouseenter is mouseleave (and the opposite of mouseout is mouseover). The enter/leave events are originally Internet Explorer-only events which jQuery has replicated, and they behave differently than the standard over/out events with regard to nested elements. jQuery's docs have a discussion of this and a demo to illustrate. It happens to work in your case, but it's a bit like saying that the opposite of multiplying is subtracting; it just happens to work out for certain numbers, but that doesn't make it right.

Next, there's the click actions. You're adding those inside the mouseenter event. This is a big no-no. It means that every time you mouse over a button, a new click event handler gets added! You just don't notice it because you're inserting and re-inserting the same element. But if you mouse over the red button 3 times before clicking it, the effect is that you're clicking it 3 times because now you've got 3 separate but identical event handlers attached.

Yet another reason to keep the actions separate from the hover effects.

You can either attach the event handlers to an element by its ID (as below), or you can use some other means of linking an element to an action.

$(".fade").hover(function () {
  $(this).fadeTo('fast', 0.4);
}, function () {
  $(this).fadeTo('fast', 1.0);
});

var hello = $("<p></p>").text("Hello").css({background: "green"});
var goodbye = $("<p></p>").text("Goodbye").css({background: "red"});

$("#helloButton").on("click", function () {
  $("#output").prepend(hello);
});

$("#goodbyeButton").on("click", function () {
  $("#output").prepend(goodbye);
});

$("#clearButton").on("click", function () {
  $("#output").empty();
});
body { background: black; }

button {
  border: none;
  padding: 1em;
}

#helloButton   { background: green; }
#goodbyeButton { background: red; }
#clearButton   { background: white; }
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

<button type="button" id="helloButton" class="fade">One</button>
<button type="button" id="goodbyeButton" class="fade">Two</button>
<button type="button" id="clearButton" class="fade">Three</button>

<div id="output"></div>

You'll notice a few things here:

  1. I'm prepending the elements to the output div, rather than inserting them next to it. This makes more sense to me since now, all the output is, well, in the output. Not beside it. And it means I can just call empty to clean up.

  2. I'm constructing the elements using jQuery rather than with strings of HTML. I generally don't like a ton of hardcoded HTML in my JS. I also don't need IDs for them, because, again, I just need to call empty.

I might go a step further and add the elements in the HTML itself, and simply hide them until needed, which would be even simpler in a lot of ways, and keep the JS free of hardcoded elements of any kind.

In the end, I'd probably replicate your code with something like this:

$("#helloButton").on("click", function () {
  $("#hello").show();
});

$("#goodbyeButton").on("click", function () {
  $("#goodbye").show();
});

$("#clearButton").on("click", function () {
  $("#output").children().hide();
});
body { background: black; }

button {
  border: none;
  padding: 1em;
  display: inline-block;
}

.fade { transition-duration: 0.5s; }
.fade:hover { opacity: 0.4; }

#helloButton   { background: green; }
#goodbyeButton { background: red; }
#clearButton   { background: white; }

#hello { background: green; }
#goodbye { background: red; }

#output p { display: none; }
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<button type="button" id="helloButton" class="fade">One</button>
<button type="button" id="goodbyeButton" class="fade">Two</button>
<button type="button" id="clearButton" class="fade">Three</button>

<div id="output">
  <p id="hello">Hello</p>
  <p id="goodbye">Goodbye</p>
</div>

It doesn't make the paragraphs switch places, but I don't think that's the most salient feature either, to be honest.

\$\endgroup\$
  • \$\begingroup\$ nice answer. since you have fade on every button, why not just add the fade style to button in css so you don't have to add the class initially on each? \$\endgroup\$ – albert Mar 15 '15 at 15:10
  • 1
    \$\begingroup\$ @albert Yeah, sorry, the answer's a bit muddled. In the last snippet, I am just adding the fade effect to all button tags, though the HTML also gives them the (pointless) fade class. I'll clean that up. But really, either things works for this, because there are only those buttons. In an actual project though, I would stick to using a class, rather than assume that every button anywhere should fade - that's too big a generalization. All buttons may fade, but not everything that fades is necessarily a button - or vice versa. The point of classes is that you can mix styles. \$\endgroup\$ – Flambino Mar 15 '15 at 15:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.