patternjavascriptMinor
Resizing a sidebar
Viewed 0 times
resizingsidebarstackoverflow
Problem
This is really simple but I would like to improve the efficiency as it's run frequently on the client:
function resizeSidebar() {
var h_tmp = 0;
if ($("#internal-holder").length > 0)
{ h_tmp = 250; }
else
{ h_tmp = 181; }
var h_var = $(window).height() - h_tmp;
var h_fixed = $(window).height();
$("#aside-holder").css("height", h_fixed - 225 + "px");
$("#main-content").css("height", h_var + "px");
$("#main-inner").css("height", h_var - 115 + "px");
$("#main-inner-scroll .viewport").css("height", h_var - 110 + "px");
$("#aside-holder-scroll .viewport").css("height", h_fixed - 225 + "px");
$("#main-inner-scroll").tinyscrollbar();
if (hideAsideScrollBar != "true") {
$("#aside-holder-scroll").tinyscrollbar();
}
}Solution
-
-
-
-
Get the reference to the elements once, not every time the function is called.
Other notes:
-
Don't do that weird thing with the braces on your
-
Use the
-
This appeases linters and reduces minified file size.
-
It also pretty much forces you to declare all your variables at the top of the function, which is generally considered to be a good practice.
-
Avoid the use of
-
Except for variables or properties representing constants. In these cases the UPPER_CASED words are joined by underscores.
-
An exception may be made for variables or properties meant to be considered "private". In these cases the camelCased words are prefixed or suffixed with an underscore.
I can almost guarantee this will be at least 10 times faster if you fix the
BUT.
What you are doing feels really wrong. Using javascript to get and set a bunch of heights should be avoidable. Again I can't say exactly what to do without seeing the markup, but you should be able to achieve most (if not all) of the
$("#foo") is slow, use the DOM: document.getElementById("foo").-
$("#foo .bar") is slow, get rid of that by navigating the DOM. I can't say exactly how without seeing the markup, but look at the FIXME comments below. -
$("#foo").css() is slow, use the DOM: foo.style.height = "24px".-
Get the reference to the elements once, not every time the function is called.
Other notes:
-
Don't do that weird thing with the braces on your
if statements.-
Use the
var keyword as little as possible by combining variable declarations.-
This appeases linters and reduces minified file size.
-
It also pretty much forces you to declare all your variables at the top of the function, which is generally considered to be a good practice.
-
Avoid the use of
_ in variable names; prefer camelCase instead. Also try to use meaningful variable names.-
Except for variables or properties representing constants. In these cases the UPPER_CASED words are joined by underscores.
-
An exception may be made for variables or properties meant to be considered "private". In these cases the camelCased words are prefixed or suffixed with an underscore.
var internalHolder = document.getElementById("internal-holder"),
asideHolder = document.getElementById("aside-holder"),
mainContent = document.getElementById("main-content"),
mainInner = document.getElementById("main-inner"),
mainInnerScroll = document.getElementById("main-inner-scroll"),
asideHolderScroll = document.getElementById("aside-holder-scroll"),
mainInnerScrollViewport = mainInnerScroll.children[0], // FIXME
asideHolderScrollViewport = asideHolderScroll.children[0]; // FIXME
function resizeSidebar() {
var tmpHeight = internalHolder.children.length ? 250 : 181,
height = window.innerHeight,
innerHeight = height - tmpHeight;
asideHolder.style.height = height - 225 + "px";
mainContent.style.height = innerHeight + "px";
mainInner.style.height = innerHeight - 115 + "px";
mainInnerScrollViewport.style.height = innerHeight - 110 + "px";
asideHolderScrollViewport.style.height = height - 225 + "px";
$(mainInnerScroll).tinyscrollbar();
if (hideAsideScrollBar != "true") {
$(asideHolderScroll).tinyscrollbar();
}
}I can almost guarantee this will be at least 10 times faster if you fix the
FIXMEs.BUT.
What you are doing feels really wrong. Using javascript to get and set a bunch of heights should be avoidable. Again I can't say exactly what to do without seeing the markup, but you should be able to achieve most (if not all) of the
$('#foo').css stuff through plain old HTML/CSS. Try giving things 100% height, try inline-block and vertical-align, hell, try tables, but javascript css styling should, IMO, be kept to an absolute minimum.Code Snippets
var internalHolder = document.getElementById("internal-holder"),
asideHolder = document.getElementById("aside-holder"),
mainContent = document.getElementById("main-content"),
mainInner = document.getElementById("main-inner"),
mainInnerScroll = document.getElementById("main-inner-scroll"),
asideHolderScroll = document.getElementById("aside-holder-scroll"),
mainInnerScrollViewport = mainInnerScroll.children[0], // FIXME
asideHolderScrollViewport = asideHolderScroll.children[0]; // FIXME
function resizeSidebar() {
var tmpHeight = internalHolder.children.length ? 250 : 181,
height = window.innerHeight,
innerHeight = height - tmpHeight;
asideHolder.style.height = height - 225 + "px";
mainContent.style.height = innerHeight + "px";
mainInner.style.height = innerHeight - 115 + "px";
mainInnerScrollViewport.style.height = innerHeight - 110 + "px";
asideHolderScrollViewport.style.height = height - 225 + "px";
$(mainInnerScroll).tinyscrollbar();
if (hideAsideScrollBar != "true") {
$(asideHolderScroll).tinyscrollbar();
}
}Context
StackExchange Code Review Q#8819, answer score: 5
Revisions (0)
No revisions yet.