3
\$\begingroup\$

I'm very much a beginner and have put together some JavaScript to control a pair of radio buttons which essentially behave as links.

I'm doing this as sort of a self-initiated project, or bit of fun. I know I don't have to use radio buttons, but it's something I just wanted to do.

It simply checks the radio button when you hover over it, and at the same time highlights the accompanying text. Or, highlights the text when hovered over, and also checks the accompanying radio button.

When the cursor moves out, no longer hovering over, the elements go back to their normal states.

I feel like there has to be a much cleaner way of implementing this.

Here is my mark up of the two radio buttons:

<form name=links>
    <div id="aaa">
        <input type="radio" name="fb" onMouseOut="out_event_01()" onMouseOver="over_event_01()" />
        <br /><span class="sl" id="fb_link" onMouseOut="out_event_03()" onMouseOver="over_event_03()">Button1</span>
    </div>
    <div id="bbb">
        <input type="radio" name="tw" onMouseOut="out_event_02()" onMouseOver="over_event_02()" />
        <br /><span class="sl" id="tw_link" onMouseOut="out_event_04()" onMouseOver="over_event_04()">Button2</span>
    </div>
</form>

As you can see I have a lot of onMouseOut and onMouseOver events.

This is the JavaScript I'm using:

function over_event_01()
{
var links = document.links.fb;
links.checked = true;
document.getElementById('fb_link').style.color = 'black';
}
function out_event_01()
{
var links = document.links.fb;
links.checked = false;
document.getElementById('fb_link').style.color = 'rgb(153,153,153)';
}
function over_event_02()
{
var links = document.links.tw;
links.checked = true;
document.getElementById('tw_link').style.color = 'black';
}
function out_event_02()
{
var links = document.links.tw;
links.checked = false;
document.getElementById('tw_link').style.color = 'rgb(153,153,153)';
}
function over_event_03()
{
var links = document.links.fb;
links.checked = true;
document.getElementById('fb_link').style.color = 'black';
}
function out_event_03()
{
var links = document.links.fb;
links.checked = false;
document.getElementById('fb_link').style.color = 'rgb(153,153,153)';
}
function over_event_04()
{
var links = document.links.tw;
links.checked = true;
document.getElementById('tw_link').style.color = 'black';
}
function out_event_04()
{
var links = document.links.tw;
links.checked = false;
document.getElementById('tw_link').style.color = 'rgb(153,153,153)';
}
\$\endgroup\$
  • \$\begingroup\$ Have you looked at JQuery? You could probably right this in two or three lines of code... \$\endgroup\$ – Steve -Cutter- Blades Oct 4 '11 at 10:19
  • \$\begingroup\$ No i haven't, I'm very new to all this. Thank though, I'll check out JQuery \$\endgroup\$ – jakry001 Oct 4 '11 at 10:43
  • \$\begingroup\$ @jakry001 Learn your JavaScript fundamentals before diving into jQuery. They'll help you write much better jQuery code. \$\endgroup\$ – bittersweetryan Oct 4 '11 at 12:45
  • \$\begingroup\$ plus many more lines of jquery core :) \$\endgroup\$ – naveen Oct 5 '11 at 9:23
6
\$\begingroup\$

Without re-writing all your code I do have a few suggestions for you:

  • Don't use inline javascript (onMouseeOut, onMouseOver) always use event listeners (I'll show you how the code example below). The reason for this is twofold: first it separates your javascript from your HTML second it makes your code even more maintainable if you change a listener you have one place to look.
  • Use classes instead of inline styles (for the same reasons mentioned above)
    • this means replace the style=xxx with a css classname (again i'll have an example in the code below)
  • A lot of your code can be condensed into small reuseable functions since a lot of it is repetitive.

Since you are new to JavaScript I'll avoid some of of the intermediate patterns that I think may apply here such as wrapping this entire functionality in an instantly invoked function expression (IIFE) but you really should read this article by Ben Alman http://benalman.com/news/2010/11/immediately-invoked-function-expression/

So lets look at how it can be improved with some psudo code. First Lets create one wrapper function that will encapsulate all your functionality. First set some function variables to your links and radios. Then we attach events for most browsers and older versions of IE, then we create two generic functions to handle your mouseenter and mouseleave. These functions will handle the duplicated logic of checking

function overOut(){
 //these variables will be accessable by all the inner functions so we only declare them once
  var fbLinks = document.links.fb,
      twLinks = document.links.tw
      fbLink = document.getElementById('fb_link'),
      twLink = document.getElementById('tw_link');

  //check for standard event listener    
  if(fbLink.addEventListener){
    //here i'm assuming that fblinks are the links you want to highlight
    fbLink.addEventListener("mouseover",radioOverFunction(this,fbLinks));
  }
  //ie events
  else if(fbLink.attachEvent){
    //here i'm assuming that fblinks are the links you want to highlight
    fbLink.attachEvent("mouseover",radioOverFunction(this,fbLinks));
  }

  /***************************************************
  *  repeat the above code but for mouseout events   *
  ***************************************************/

  var radioOverFunction = function(radio,target){
      //in here you would set check the radio and set the classname on the target
      target.className = 'mouseOverClassName';
  };

  var radioLeaveFunction = function(radio,target){
      //in here you would set check the radio and set the classname on the target
      target.className = 'standardClassName';
  }


}
\$\endgroup\$
5
\$\begingroup\$

You could probably start by indenting your code.

My Javascript is a bit rusty, but I would suggest you to rewrite your code so it uses only one function with parameters, instead of 8 functions which all do pretty much the same.

function hover_link(is_over, is_fb)
{
    var links
    if (is_fb)
        links = document.links.fb;
    else  // tw
        links = document.links.tw;
    links.checked = true;
    if (is_over)
        document.getElementById('tw_link').style.color = 'black';
    else  // out
        document.getElementById('fb_link').style.color = 'rgb(153,153,153)';
 }

You would use it like this:

<form name=links>
    <div id="aaa">
        <input type="radio" name="fb" onMouseOut="hover_link(false, true)" onMouseOver="hover_link(true, true)" />
        <br /><span class="sl" id="fb_link" onMouseOut="hover_link(false, false)" onMouseOver="hover_link(true, false)">Button1</span>
    </div>
    <div id="bbb">
        <input type="radio" name="tw" onMouseOut="hover_link(false, true)" onMouseOver="hover_link(true, true)" />
        <br /><span class="sl" id="tw_link" onMouseOut="hover_link(false, false)" onMouseOver="hover_link(true, false)">Button2</span>
    </div>
</form>
\$\endgroup\$

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.