Somebody please verify this code. Is this efficient? please tell me modular way to accomplish this task.
Updated
I want to add
show
class todata-target
and removeshow
class fromdata-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.
-
\$\begingroup\$ Does it work as intended? \$\endgroup\$Mast– Mast ♦2016年05月25日 09:37:32 +00:00Commented May 25, 2016 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– Vogel6122016年05月25日 09:37:48 +00:00Commented May 25, 2016 at 9:37
-
\$\begingroup\$ @Mast Yes working fine. \$\endgroup\$Asfath Ahamed– Asfath Ahamed2016年05月25日 09:54:16 +00:00Commented May 25, 2016 at 9:54
-
\$\begingroup\$ @Vogel612 Unable to edit the post. \$\endgroup\$Asfath Ahamed– Asfath Ahamed2016年05月25日 09:58:36 +00:00Commented May 25, 2016 at 9:58
-
\$\begingroup\$ @user5827047 Just click the edit link. \$\endgroup\$BCdotWEB– BCdotWEB2016年05月25日 10:52:02 +00:00Commented May 25, 2016 at 10:52
2 Answers 2
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() { ... });
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.
-
\$\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 thedocument
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\$Greg Burghardt– Greg Burghardt2016年05月26日 13:54:51 +00:00Commented May 26, 2016 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– Greg2016年05月26日 14:41:53 +00:00Commented May 26, 2016 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
anddata-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\$Greg Burghardt– Greg Burghardt2016年05月26日 15:01:46 +00:00Commented May 26, 2016 at 15:01
Explore related questions
See similar questions with these tags.