8
\$\begingroup\$

I would like to connect this "JS" to Bugzilla (example: bugzilla.mozilla.org or landfill.bugzilla.org).

I started to learn JS language today and I would like to ask you:

  1. How can I not do bad things in global scope?
  2. How should I use functions (in var or not)?

quicksearch = document.getElementById('quicksearch_top');
comment = document.getElementById('comment');

severity = document.getElementById('bug_severity');
priority = document.getElementById('priority');

commit_top = document.getElementById('commit_top');
commit = document.getElementById('commit'); 

function focusonload() { 
// may be it should be
// in next function?

    function cursorfocus(s) {
        x = window.scrollX;
        y = window.scrollY;
        s.focus();
        window.scrollTo(x, y);
    }

    if (comment !== null) {
        cursorfocus(comment)
    } else {
        quicksearch.focus();
    }
}

function navigation(keypressed) { 

    keypressed = keypressed || window.event;

    function selectelement(w, select) {
        w.value = select;
    }

    keyCode = keypressed.keyCode || keypressed.which,
    kn = {
        enter: 13, 
        save: 83, 
        down: 40, 
        up: 38, 
        p1: 49, 
        p2: 50, 
        p3: 51, 
        p4: 52, 
        p5: 53, 
    };

    if (keypressed.altKey) {
        if (keyCode == nk.save && commit_top!==null){
            commit_top.click();
        }

        var key_str;
        if (priority!==null){
            switch (keyCode) {
                case kn.p1: key_str = "P1"; break;
                case kn.p2: key_str = "P2"; break;
                case kn.p3: key_str = "P3"; break;
                case kn.p4: key_str = "P4"; break;
                case kn.p5: key_str = "P5"; break;
            }
        }
        selectelement(priority, key_str);
        severity.focus();

    } else if (keypressed.ctrlKey) {
        switch (keyCode) {
            case kn.enter:
                if (commit!==null&&comment.value!=="") {
                    commit.click();
                }
                break;
            case kn.up:
                if (quicksearch!==null) {
                    quicksearch.focus();
                }
                break;
            case kn.down:
                if (comment!==null) {
                    comment.focus();
                }
                break;
        }
    }
}

focusonload();
document.onkeydown = navigation;
// how to call this functions when page
// would be loaded at all (with window.onload)
\$\endgroup\$
  • \$\begingroup\$ typo: if (keyCode == nk.save && commit_top!==null){ should be kn.save not nk.save? \$\endgroup\$ – James Khoury Feb 4 '14 at 1:29
10
\$\begingroup\$

How can I not mess up the global scope?

You can use an IIFE ( Immediately Invoked Function Expression ) to surround your code and then assign all your variables with var <- Very important.

(function(){

  var quicksearch = document.getElementById('quicksearch_top');
  var comment = document.getElementById('comment');

  var severity = document.getElementById('bug_severity');
  var priority = document.getElementById('priority');

  var commit_top = document.getElementById('commit_top');
  var commit = document.getElementById('commit'); 

  //the rest of your code here

}()); 

How should I use functions (in var or not)?

It really is up to you, at this point I prefer function.

Other than that:

You should use lowerCamelCasing, so

  • commit_top -> commitTop
  • quicksearch -> quickSearch
  • selectelement -> selectElement

etc.etc

Your keycode map is very nice, keep it that way!

One last comment is that you should not use document.onkeydown = navigation;, there is a good chance that you will break something this way.

Try document.addEventListener("keydown", navigation, true); instead.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your advices and for being answered. \$\endgroup\$ – dmgl Jan 31 '14 at 11:27
3
\$\begingroup\$

To add a couple of points to konijns answer;

You can remove the need for verbose switch statements with a dict:

var kn = {
    49: 'P1', 
    50: 'P2', 
    51: 'P3', 
    52: 'P4', 
    53: 'P5', 
};

...

var keyStr = kn[keyCode];

You can call a function when the page loads using the onload event:

window.onload = focusOnLoad;

Since it's only executed once function cursorfocus(s) may as well be inline, rather than a function declaration. The same with selectElement().

I also like to put the larger block of code in my else statement. This makes it easier to read, and the flow feels better. Also avoid unnecessary variables, it's quite clear what this code means without x and y.

if (!comment) {
  quicksearch.focus();
} else {
    s.focus();
    window.scrollTo(window.scrollX, window.scrollY);
}

Finally, don't forget those var statements!

\$\endgroup\$
  • \$\begingroup\$ Why would you use an anon functionwindow.onload = function() { for onload when you're just calling a single function? Also I on't think the focusOnLoad() function was specified as on load of window but rather on load of the script. Also the difference between your kn object and the OP's is that yours is in reverse. (numbers to names rather than names to numbers) \$\endgroup\$ – James Khoury Feb 4 '14 at 1:06
  • \$\begingroup\$ @JamesKhoury Whoops, good point. I got the onload requirement from the comments at the bottom. Quite true about the kn object, but since he's not really using the names for anything I deemed the object mapping more effective. \$\endgroup\$ – Jivings Feb 4 '14 at 1:19
  • \$\begingroup\$ the kn object is used in detirmining the keyCode \$\endgroup\$ – James Khoury Feb 4 '14 at 1:29
  • \$\begingroup\$ @JamesKhoury Yes, but the keys in the kn object aren't used for anything, so they don't need to be words like enter, save and down, it's simple enough to understand without. \$\endgroup\$ – Jivings Feb 4 '14 at 14:21

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.