2
\$\begingroup\$

I have just started learning about JavaScript and events. I am interested in learning the JavaScript and not jQuery until I am better at JavaScript.

I wrote this code that will show and hide my div because I basically want to be able to use this code in future web development projects but at the moment it will only apply to one element on page but what if I want to have a few pop up boxes. I understand I will have to have the element positioned absolute.

Can I also ask anyone that is good at JavaScript events to check my code to see if it is well written or if it could be improved? I am just a learning.

To make my questions clearer:

  1. How would I make this code more usable for more than one element?
  2. Is my code well written or does it look like a noob wrote it?
<style>
    a#button{
        background-color: #FFFF00;
        display:block;
    }

    div#menu{
        background-color: #000000;
        color: #ffffff;
        display:none;
    }
</style>

<body>
<a id="button" href="#">Click Me</a>
<div id="menu">SUPRISE</div>

<script>
    document.addEventListener("click", whereWasClicked, false); // listener for any clicks on the document.

var button = document.getElementById("button");
button.addEventListener("click", displaySuprise, false);


var clicked;
function whereWasClicked(){
    clicked = event.target;
    //check to see if user clicks outside of the
    if (tog = 1 && clicked != menu) {
            menu.style.display = "none";
            document.getElementById("button").innerHTML = "Click Me";
            tog = 0;
        }
}
var menu = document.getElementById("menu");
var tog = 0; // set up a toggle so you can display block and then none.
function displaySuprise(){
    event.preventDefault();
    event.stopPropagation();
    if(tog == 0){
        menu.style.display = "block";
        this.innerHTML = "Opened!";
        tog = 1;
    } else {
        menu.style.display = "none";
        this.innerHTML = "Click Me";
        tog = 0;
    }
    // check to see if the user has clicked anywhere off the new element.

}
</script>
\$\endgroup\$
  • \$\begingroup\$ My initial thought is -- you are really doing great. Doesn't look like NOOBs code. Think of using prototype to improve modularity of the code. \$\endgroup\$ – asyncwait Jan 7 '12 at 19:43
  • \$\begingroup\$ jsfiddle.net/uPwNy/14 \$\endgroup\$ – Raynos Jan 7 '12 at 20:33
  • \$\begingroup\$ @Raynos when the user clicks away from the element the element should disappear. \$\endgroup\$ – jamcoupe Jan 7 '12 at 21:29
  • \$\begingroup\$ @jamcoupe I know, that's bad design. I fixed it for you. Now when you click the button again it hides itself. \$\endgroup\$ – Raynos Jan 7 '12 at 22:50
3
\$\begingroup\$

Item 1: I'd suggest getting rid of all global variables except tog. They don't look like they are needed and just open you up for a potential conflict with other global variables.

Item 2: This line of code:

if (tog = 1 && clicked != menu) {

should probably be:

if (tog == 1 && clicked != menu) {

Comparisons are done with == or ===, not with = which is an assignment.

Item 3: addEventListener() does not exist in versions of IE before IE9, so you will need to use attachEvent() if addEventListener() is not present.

Item 4: Here's a version I've done that gets rid of all globals except tog (which I renamed menuVisble and made it a boolean. It also uses anonymous functions (less global namespace pollution) for the handlers and makes some other cleanups. You can see it work here: http://jsfiddle.net/jfriend00/MEaPs/.

var menuVisible = false; // set up a toggle so you can display block and then none.

// listener for any clicks on the document.
document.addEventListener("click", function(e) {
    var clicked = e.target;
    var menu = document.getElementById("menu");
    //check to see if user clicks outside of the
    if (menuVisible && clicked != menu) {
            menu.style.display = "none";
            document.getElementById("button").innerHTML = "Click Me";
            menuVisible = false;
        }

}, false); 

document.getElementById("button").addEventListener("click", function(e) {
    var menu = document.getElementById("menu");
    e.preventDefault();
    e.stopPropagation();
    if (menuVisible){
        menu.style.display = "none";
        this.innerHTML = "Click Me";
        menuVisible = false;
    } else {
        menu.style.display = "block";
        this.innerHTML = "Opened!";
        menuVisible = true;
    }
}, false);

Item 5: And here's a version with no globals at all. It uses the class of the menu to control and discern visibility. You can see it work here: http://jsfiddle.net/jfriend00/96NfD/.

// listener for any clicks on the document.
document.addEventListener("click", function(e) {
    var menu = document.getElementById("menu");
    var menuVisible = menu.className.indexOf("hidden") == -1;
    //check to see if user clicks outside of the
    if (menuVisible && e.target != menu) {
            document.getElementById("button").innerHTML = "Click Me";
            menu.className = "hidden";
        }

}, false); 

document.getElementById("button").addEventListener("click", function(e) {
    e.preventDefault();
    e.stopPropagation();
    var menu = document.getElementById("menu");
    var menuVisible = menu.className.indexOf("hidden") == -1;
    if (menuVisible) {
        this.innerHTML = "Click Me";
        menu.className = "hidden";
    } else {
        this.innerHTML = "Opened!";
        menu.className = "";
    }
}, false);
\$\endgroup\$
  • \$\begingroup\$ Thanks for the tips! I know about the pre IE9 problem but I am not going to concern myself over it until I need to (when I am making proper websites.) \$\endgroup\$ – jamcoupe Jan 7 '12 at 19:49
  • \$\begingroup\$ It seems that I have to have tog as a global variable for it to toggle properly \$\endgroup\$ – jamcoupe Jan 7 '12 at 19:51
  • \$\begingroup\$ @jamcoupe - Yes, that's why I said to "get rid of all global variables except tog". You wouldn't technically need tog either if you just examine the object to see if it's visible or not, but that would require writing a little more code. \$\endgroup\$ – jfriend00 Jan 7 '12 at 20:02
  • \$\begingroup\$ @jamcoupe - I added two new versions of the code for you to look at. \$\endgroup\$ – jfriend00 Jan 7 '12 at 20:23
  • \$\begingroup\$ your code looks much better and I haven't looked into .className yet. but I understand how your code is working! Thanks for your help \$\endgroup\$ – jamcoupe Jan 7 '12 at 21:30
1
\$\begingroup\$

jfriend00's answer is great.

And I think you can rewrite your click-outside function like this for llater reuse.

var clickOutside = function(element, action) {
  return function(e) {
    action(e.target !== element);
  };
};

document.addEventListener(
  "click",
  clickOutside(menu, function(isOutside) {
    if(isOutside && menuVisible) {
      // hide your menu
    }
  }),
  false
);
\$\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.