3
\$\begingroup\$

What is the best way to refactor the following script?

<script type="text/javascript">

    $(document).ready(function() {
        $(".rolesList").hide(); 
    });

        $("#legendFunction").click(function() {
        $("#divChkUserRoles").toggle('slow');            
            var text = $("#lblExpandFunction").text() == '+' ? '-' : '+';
            $("#lblExpandFunction").text(text);
        })

        $("#legendMISheet").click(function() {
        $("#divChkUserRolesMISheet").toggle('slow');
            var text = $("#lblExpandMISheet").text() == '+' ? '-' : '+';
            $("#lblExpandMISheet").text(text);
        })

</script>

The above script will be applied to the following HTML and ASP.NET code:

            <fieldset>
                <legend><span id="legendFunction" style="cursor: pointer" title="Click here to toggle show or collapse Function.">
                    <label id="lblExpandFunction" style="padding: 5px;">+</label>
                    KengLink Function 
                </span></legend>
                <div id="divChkUserRoles"  class="rolesList">      
                    <asp:CheckBoxList ID="UserRoles" runat="server" />                  
                </div>
            </fieldset>
            <fieldset>
                <legend><span id="legendMISheet" style="cursor: pointer" title="Click here to toggle show or collapse MI Sheet.">
                    <label id="lblExpandMISheet" style="padding: 5px;">+</label>
                MI Sheet
                </span></legend>
                <div id="divChkUserRolesMISheet" class="rolesList">  
                    <asp:CheckBoxList ID="UserRolesMISheet" runat="server" />
                </div>
            </fieldset>
\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

Don't use ids at all. (You almost never should be using them. Ids get to be really problematic once you have composite views and/or multiple people on a project)

Place a few appropriate classes in your html. And use the jquery composite and relative references to do it all at once.

$('.expander').click(function() {
  $(this).closest('fieldset').find('.expandable').toggle('show');
  var l = $(this).find('label');
  l.text(l.text() === '+' ? '-' : '+');
});

where your clicker span gets a class of expander and your rolesList gets the class of expandable. You could just use rolesList directly, but I find expandable to be somewhat more descriptive.

\$\endgroup\$
3
  • \$\begingroup\$ Skillful solution, anyway, Why class not be a problematic as id too. \$\endgroup\$ Mar 27, 2012 at 15:28
  • 1
    \$\begingroup\$ @SarawutPositwinyu many libraries assume that only one instance of an id exists on a page. Once you start composing views or having multiple people its too easy to put several of the same ids on one page and get really weird errors. With classes you have to assume that there is more than one and you have to take that into account. Also, the fact that you can have multiple classes on an instance usually guides people to give things more meaningful names. \$\endgroup\$ Mar 27, 2012 at 19:49
  • \$\begingroup\$ Thx, Oh class name should be '.expandable' instead of 'expandable' (in the code). \$\endgroup\$ Mar 28, 2012 at 3:22
0
\$\begingroup\$

In addition to George solution, you can also use on()/off() event API rather then using click(), on/off much faster then other jQuery event handlers.

And rather then calling anonymous function inside click(), use named function to debug it easily later on, it's always better to write the JavaScript code in chunks/modules for the sake of modularity.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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