patternjavascriptMinor
Hide / Show panels depending of value of a dropdown
Viewed 0 times
showdropdowndependingvaluepanelshide
Problem
According to the value of a dropdown menu I have to show/hide two panels.
For option 'derive' and 'reject' I should show the comment panel, and just for the 'derive' panel I should also show a derivation panel. There are other
This works, but how could I refactor the code to make it shorter and more efficient without losing clarity?
For option 'derive' and 'reject' I should show the comment panel, and just for the 'derive' panel I should also show a derivation panel. There are other
This works, but how could I refactor the code to make it shorter and more efficient without losing clarity?
$("#formTabs").on( "change", "select.actions",function(){
var parentContainer = $(this).closest("fieldset");
if(this.value == "reject" || this.value == "derive"){
parentContainer.find(".commentPanel").show();
}
// other 3 options where the panel should not show up.
else {
parentContainer.find(".commentPanel").hide();
}
// Just for the derive option, a derivation panel must be shown.
if(this.value == "derive"){
parentContainer.find(".derivationPanel").show();
} else {
parentContainer.find(".derivationPanel").hide();
}
});Solution
The only "efficiency" improvement I see is caching the result of the
on the other hand it's good to reduce the number of duplicated literals.
so it would be good to eliminate those too.
Lastly, I would recommend some minor style improvements:
Putting it together:
Note that by extracting the
this.value == "derive" check to avoid evaluating the string comparison twice. The effect of this is close to nothing,on the other hand it's good to reduce the number of duplicated literals.
".commentPanel" and ".derivationPanel" are also duplicated literals,so it would be good to eliminate those too.
Lastly, I would recommend some minor style improvements:
- Don't put a space between
( "change"
- Put a space after the comma in
"select.actions",function
- Put a space between
){infunction(){, everywhere
Putting it together:
$("#formTabs").on("change", "select.actions", function() {
var parentContainer = $(this).closest("fieldset");
var commentPanel = parentContainer.find(".commentPanel");
var derivationPanel = parentContainer.find(".derivationPanel");
var isDerive = this.value == "derive";
if (this.value == "reject" || isDerive) {
commentPanel.show();
} else {
// other 3 options where the panel should not show up.
commentPanel.hide();
}
// Just for the derive option, a derivation panel must be shown.
if (isDerive) {
derivationPanel.show();
} else {
derivationPanel.hide();
}
});Note that by extracting the
commentPanel, derivationPanel, isDerive variables you don't lose any efficiency at all, as all the right-hand sides would be evaluated in the code no matter what. This rewritten version is better, because of the string literals now appear only once, which eliminates typos, and if you need to change something, you can do it at a single place instead of two as in the original.Code Snippets
$("#formTabs").on("change", "select.actions", function() {
var parentContainer = $(this).closest("fieldset");
var commentPanel = parentContainer.find(".commentPanel");
var derivationPanel = parentContainer.find(".derivationPanel");
var isDerive = this.value == "derive";
if (this.value == "reject" || isDerive) {
commentPanel.show();
} else {
// other 3 options where the panel should not show up.
commentPanel.hide();
}
// Just for the derive option, a derivation panel must be shown.
if (isDerive) {
derivationPanel.show();
} else {
derivationPanel.hide();
}
});Context
StackExchange Code Review Q#68103, answer score: 2
Revisions (0)
No revisions yet.