5
\$\begingroup\$

I just finished creating a fancy checkbox and am hoping for some tips to help improve my code, as I am still learning.

$(document).ready(function(){
  function checkbox(){
    var y = $("#yon");
    var n = $("#non");
    var cx = $("#chex")
    var cbx = $("#checkbox");
    var chval = false;
    $(".cspan").click(function(){
      $(n).toggleClass("no");
      $(y).toggleClass("yes");
      if(chval){
        $(cx).text("\u2716");
        $(cbx).prop('checked', false);
        chval = false;
      }
      else{
        $(cx).text("\u2714");
		$(cbx).prop('checked', true);
        chval = true;
      }
    });
  }
  checkbox();
});
#checkcon p{

}
input[type="checkbox"] {
    display:none;
}
input[type="checkbox"] + label span {
    display:inline-block;
	width: 160px;
	height: 60px;
	border-radius: 25px;
    margin:-1px 4px 0 0;
    vertical-align:middle;
    background-color: #ff3a31;
    cursor:pointer;
	transition: background-color 100ms linear;
	position:absolute;
}
input[type="checkbox"]:checked + label span {
    background-color: #00cc66;
    transition: background-color 100ms linear;

}
input[type="checkbox"] {
	
}
.checkemb{
	position:relative;
	display:inline-block;
	margin-right:10px;
	margin-left:10px;
	font-family:"Arial Black", Gadget, sans-serif;
	color:#ffffff;
}
#chex{
	margin-right:25px;
	margin-left:25px;
	
}
p.yes{
	color:#990800;
	transition: color 100ms linear;
}
p.no{
	color:#008040;
	transition: color 100ms linear;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<body>
  <div id="checkcon">
    <input id="checkbx" type="checkbox"/>
    <label for="checkbx"><span class="cspan"></span></label>
    <p class="checkemb yes" id="yon">Yes</p><p class="checkemb x" id="chex"><strong>&#10006 </strong></p><p class="checkemb" id="non">No</p>		
  </div>
</body>

\$\endgroup\$
0

3 Answers 3

Reset to default
3
\$\begingroup\$

A few points:

  • You don't need to declare the checkbox function within the $(document).ready(), you can do it outside it.
  • You have some empty CSS rules -- by accident?
  • $(function() {...}); is a shorthand for $(document).ready(function() {...}); -- you might want to use that if you want
  • You don't need to do $(y), $(n), because you saved y, n as jQuery objects already
  • When you make the variables y, n, cx, cbx, you can opt to (at the end of the day, it's personal preference, but makes it clear they are 'special' variables) prepend $ (or jQuery) like var $y = $("#yon"), var $n = $("#non"), and so on..., so you know they're jQuery objects.

With that in mind...:

function checkbox() {
  var $y = $("#yon");
  var $n = $("#non");
  var $cx = $("#chex")
  var $cbx = $("#checkbox");
  var chval = false;
  $(".cspan").click(function() {
    $n.toggleClass("no");
    $y.toggleClass("yes");
    if (chval) {
      $cx.text("\u2716");
      $cbx.prop('checked', false);
      chval = false;
    } else {
      $cx.text("\u2714");
      $cbx.prop('checked', true);
      chval = true;
    }
  });
}

$(function() {
  checkbox();
});
input[type="checkbox"] {
  display: none;
}
input[type="checkbox"] + label span {
  display: inline-block;
  width: 160px;
  height: 60px;
  border-radius: 25px;
  margin: -1px 4px 0 0;
  vertical-align: middle;
  background-color: #ff3a31;
  cursor: pointer;
  transition: background-color 100ms linear;
  position: absolute;
}
input[type="checkbox"]:checked + label span {
  background-color: #00cc66;
  transition: background-color 100ms linear;
}
.checkemb {
  position: relative;
  display: inline-block;
  margin-right: 10px;
  margin-left: 10px;
  font-family: "Arial Black", Gadget, sans-serif;
  color: #ffffff;
}
#chex {
  margin-right: 25px;
  margin-left: 25px;
}
p.yes {
  color: #990800;
  transition: color 100ms linear;
}
p.no {
  color: #008040;
  transition: color 100ms linear;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>

<body>
  <div id="checkcon">
    <input id="checkbx" type="checkbox" />
    <label for="checkbx"><span class="cspan"></span>
    </label>
    <p class="checkemb yes" id="yon">Yes</p>
    <p class="checkemb x" id="chex"><strong>&#10006 </strong>
    </p>
    <p class="checkemb" id="non">No</p>
  </div>
</body>

\$\endgroup\$
1
\$\begingroup\$

There's a bug:

The checkbox works for all areas except where there's text. Yes, X and No are not click-able.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ This is not really a code review, perhaps this should be a comment instead? \$\endgroup\$
    – Phrancis
    Commented Apr 2, 2016 at 19:44
  • 3
    \$\begingroup\$ @Phrancis I'd say it's important enough to be an answer since comments aren't forever. There's an edge case he didn't cover and I'm pointing it out. \$\endgroup\$
    – Mast ♦
    Commented Apr 2, 2016 at 19:45
1
\$\begingroup\$

On your css:

  1. Don't use IDs, they aren't necessary in css and are hard to over-ride if you want to modify the css somewhere else. IDs are OK for hooking in JS functionality as it doesn't have specificity concerns.
  2. Don't over-qualify your selectors. There's no need to do p.some-class when .some-class will do - this is useful because it doesn't tie your styles to your HTML and it can be used on any element.
  3. Consider separating out the classes you use for styling and the classes you use to hook in JS functionality. The reason for this is makes it easier to switch out style or functionality without affecting the other.
  4. Some of your class names are not clear due to abbreviation, using a few extra characters to convey clear meaning is better than saving those few characters for efficiency.

One thing I also think is important that isn't covered by other answers is the use of anonymous functions. They're a great way to build code up quickly, but there's a few problems with them.

  1. They aren't particularly clear, i.e. if you separate it out and give it a good name the next developer can quickly assess what it does without having to look at the internals, whereas when it's an anonymous function all you see is that stuff happens. In your example, I can see a click handler, but there is probably a clear name that can be given that will quickly convey meaning to the next developer.

  2. They aren't re-usable. When your code develops and you want to call the click handling code from elsewhere (which you will do, as you are probably going to want to handle the space bar as a checkbox toggle)

    var toggleCheckbox = function() { }

    $('.selector').on('click', toggleCheckbox);

These points are more about maintainability.

One final thing that you must consider when using JS and CSS to mimic basic controls is that the browser and operating system do these things very well indeed. There are many ways to interact with form elements that have been refined over years of studies into accessibility. It's not advisable to remove a lot of that without good reason.

You might want to look at SVGs and :checked states to build it without using JS.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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