2
\$\begingroup\$

Somebody please verify this code. Is this efficient? please tell me modular way to accomplish this task.

Updated

I want to add show class to data-targetand remove show class from data-group Just like happening in an accordion. I wanted to know that this code is perfect for that scenario. I'm new here please let know if did not provide enough information.

$(function () {
  $(document).on("click", ".showHide", function() {
    var hideClasses = $(this).data("group").split(" ");
    var showClasses = $(this).data("target").split(" ");

    $.each(hideClasses, function(k, v) {
      $("."+v).removeClass("show");
    });
    $.each(showClasses, function(k, v) {
      $("."+v).addClass("show");
    });
  });  
});

HTML tag

<a href="#" class="showHide" data-target="loginPanel" data-group="accountPanel">Remeber Password?</a>

More Info

Please find the pen I have the working link. Try clicking on login, sign up, forgot password password and the stay relaxed.

CodePen Link

\$\endgroup\$
9
  • \$\begingroup\$ Does it work as intended? \$\endgroup\$
    – Mast
    May 25 '16 at 9:37
  • 2
    \$\begingroup\$ Welcome to Code Review! As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$
    – Vogel612
    May 25 '16 at 9:37
  • \$\begingroup\$ @Mast Yes working fine. \$\endgroup\$ May 25 '16 at 9:54
  • \$\begingroup\$ @Vogel612 Unable to edit the post. \$\endgroup\$ May 25 '16 at 9:58
  • \$\begingroup\$ @user5827047 Just click the edit link. \$\endgroup\$
    – BCdotWEB
    May 25 '16 at 10:52
1
\$\begingroup\$

Instead of looping over the showClasses and hideClasses you can do a regex search and replace, then add/remove the class names:

var trimRegex = /^\s+|\s+$/g,
    selectorRegex = /(^| +)([-_a-zA-Z0-9]+)/g,
    callback = function(match, $1, $2) {
        if ($1) {
            return ", ." + $2;
        } else {
            return "." + $2;
        }
    },
    hideSelector = this.getAttribute("data-group")
        .replace(trimRegex, "")
        .replace(selectorRegex, callback),
    showSelector = this.getAttribute("data-target")
        .replace(trimRegex, "")
        .replace(selectorRegex, callback);

$(hideSelector).removeClass("show");
$(showSelector).addClass("show");

Secondly, since the document object exists the moment JavaScript begins executing, there is no need to wait for the document to load:

$(document).on("click", ".showHide", function() { ... });
\$\endgroup\$
0
\$\begingroup\$

Couldn't you simply do:

function hideAndShow(element) {
    var selector = this;
    var anchor = '[data-rel="' + $(this).attr('data-rel') + '"]';
    $.each($(anchor), function (index, value) {
        if(value !== selector)) {
            if(value.is(':visible')) {
                value.hide();
            } else {
                value.show();
            }
        }
    });
}

You have a single attribute, remove an extra loop, which should boost performance a bit. Plus you would add to your markup like:

<a href="javascript(void);" data-rel="account-panel" onclick="hideAndShow(this)">Example</a>

Which should help it be a bit more expressive and easy to understand, and it isn't attached to the document click event, which is usually a no, no.

\$\endgroup\$
3
  • \$\begingroup\$ If you query based on the data-rel attribute, you will also show/hide the anchor tag as well, which I don't think is the intended behavior. Additionally, the OP's approach of attaching a click handler to to the document is perfectly fine. Event delegation is not that big of a drag on performance. When the page size grows, so do the number of event handlers with your solution. This will actually reduce performance on large pages. \$\endgroup\$ May 26 '16 at 13:54
  • \$\begingroup\$ @GregBurghardt He hasn't specified the intent well, could you provide evidence of that performance? I was taught differently, so I'd like to read about that. You can correct me if I'm wrong, but that would create a global event listener, waiting for you to click the document. Wouldn't the introduce potential for another developer to cause an issue? Why the down vote though? Based on the information provided it answers the OP's question adequately. \$\endgroup\$
    – Greg
    May 26 '16 at 14:41
  • \$\begingroup\$ In the OP's code, the anchor itself is not being toggled between visible and invisible because the classes that are toggled come from the data-group and data-target attributes. And those classes are not on the anchor tag that got clicked. Your code would behave differently because the anchor tag itself has an attribute by the same value as the target elements. \$\endgroup\$ May 26 '16 at 15:01

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.