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\$
4
  • \$\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
    Commented Jan 7, 2012 at 19:43
  • \$\begingroup\$ jsfiddle.net/uPwNy/14 \$\endgroup\$
    – Raynos
    Commented Jan 7, 2012 at 20:33
  • \$\begingroup\$ @Raynos when the user clicks away from the element the element should disappear. \$\endgroup\$
    – jamcoupe
    Commented Jan 7, 2012 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
    Commented Jan 7, 2012 at 22:50

2 Answers 2

Reset to default
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\$
5
  • \$\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
    Commented Jan 7, 2012 at 19:49
  • \$\begingroup\$ It seems that I have to have tog as a global variable for it to toggle properly \$\endgroup\$
    – jamcoupe
    Commented Jan 7, 2012 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
    Commented Jan 7, 2012 at 20:02
  • \$\begingroup\$ @jamcoupe - I added two new versions of the code for you to look at. \$\endgroup\$
    – jfriend00
    Commented Jan 7, 2012 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
    Commented Jan 7, 2012 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 and acknowledge you have read our privacy policy.

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