2
\$\begingroup\$

I'm not a great JS coder so I need your expertises to look into my code (although nothing very special and it works fine) to see if it is good practise or not because I heard that dodgy JS coding might slow down the site especially when it comes to reading mouse or user events.

function textbox_clear_default(form_field, mouse_event)
{
    var field_data = (document.getElementById(form_field).value).replace(/ /gi, '');
    var lower_case = field_data.toLowerCase();

    if (form_field == 'text_site_search')
    {
        if (lower_case == 'sitesearch')
        {
            document.getElementById(form_field).value = '';
        }
    }

    if (mouse_event == 'onclick')
    {
        if (lower_case == 'sitesearch')
        {
            document.getElementById(form_field).value = '';
        }
    }

    if (mouse_event == 'onblur')
    {
        if (lower_case == 'sitesearch' || lower_case == '')
        {
            document.getElementById(form_field).value = 'Site Search';
        }
    }
}
<input id="text_site_search" type="text" name="text_site_search" value="Site Search" onclick="textbox_clear_default(this.id, 'onclick')" onblur="textbox_clear_default(this.id, 'onblur')" />

\$\endgroup\$
1
  • 2
    \$\begingroup\$ In modern browsers you can simply use <input placeholder="Site Search">. For older browsers you might be able to use a polyfill. \$\endgroup\$ Commented Sep 6, 2013 at 9:45

2 Answers 2

2
\$\begingroup\$

This is the way I would approach it. Live demo here: http://jsbin.com/iHEgalU/3/edit Sure, bad js can slow your site down. Something like this is going to be pretty fast no matter what you do (unless you go out of your way to make it slow), but better is better and I say it's good to write the best, fastest, and most totally rock-sauce code you possibly can. So here goes.

Some people may consider it overkill considering the simplicity of your task, but I prefer writing in a manner that allows me to easily expand my script while keeping everything tidy and following good practices. You can extract the improved logic and use it in whatever style you like. I have commented this quite a bit. Refer to the live demo where there are no comments if you'd rather see it that way.

<input id="text_site_search" type="text" name="text_site_search" value="Site Search">

Avoid inline event functions! Notice that I have removed them from the html and put them in the js where they belong! This is much cleaner! HTML can't "do" things, so it doesn't need to know about click functions.

var myStuff = {
  options: { //these will be defaults, used if a parameter is not passed to init()
    sel: '',
    placeholder: 'My Placeholder'
  },
  init: function(sel, placeholder) { //this will kick everything off, that's all we need to run, you can pass options in like this or by object. I tend to use objects, but no need here.
    if (sel) { this.options.sel = sel }
    if (placeholder) { this.options.placeholder = placeholder }
    //there are several ways you could replace the passed in options...this is just an example that's ok for this case. jQuery has a method called $.extend, for example

    var obj = this; //cache a reference to "this" which currently refers to "myStuff"
    var inputSearch = document.getElementById(sel); //search the dom for the element and cache the reference
    //no need to keep searching the dom for an element. Search once and store the result.

    inputSearch.addEventListener('focus', function() {
      if (this.value === obj.defaultValue) { //"this" refers to the element when inside an event handler
        this.value = '';
      }
    });

    inputSearch.addEventListener('blur', function() {
      if (this.value === "") {
         this.value = obj.defaultValue; 
      }
    });
  }
};

myStuff.init('text_site_search', 'Site Search');

The options thing here isn't really all that helpful given the simplicity. If this is all this will ever do, I'd probably just use the parameters directly (just use sel where you need it), but I wanted to demonstrate the concept.

Note: IE8 and lower do not support addEventListener. Look up the conditional that deals with that issue if needed.

The funny thing about this whole script is that it is remarkably similar to html5 placeholders, though older browsers won't support that.

<input type="text" name="text_site_search" placeholder="Site Search">
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Other than making it a little more generic (not binding explicitly to text_site_search) I do not think that this is overkill. It's certainly less overkill than reaching for the jQuery. Could mention HTML 5 placeholders here. \$\endgroup\$
    – JohnMark13
    Commented Sep 5, 2013 at 18:27
  • 1
    \$\begingroup\$ aren't there some browser compatibility issues with addEventListener? \$\endgroup\$
    – Stuart
    Commented Sep 5, 2013 at 18:56
  • \$\begingroup\$ @JohnMark13 Oops! You know, I was meaning to make a note about options and I totally forgot. Doh. Great call. I'll update. \$\endgroup\$
    – m59
    Commented Sep 5, 2013 at 20:04
  • \$\begingroup\$ @Stuart older IE doesn't support it, but that's an easily discoverable issue (when it throws the error, lol) and I hate to ugly up my sample code with fail browser workarounds. Anyway, good note. I'll add something in my answer. \$\endgroup\$
    – m59
    Commented Sep 5, 2013 at 20:05
0
\$\begingroup\$

I'll just be short and blunt because I'm on a mobile phone:

Your JS solution is not reusable and quite naive. Consider for instance a user tabbing into your input.

Please just use the HTML placeholder attribute, it is meant for that.

\$\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.