2
\$\begingroup\$

Inspired by this website, I wrote the following function to automatically generate a div and load a page for each page listed in an array of page names. The idea is that if I want to conveniently display webpages or other pre-packaged content, I just need to make an array of the file names/URLs and then the function creates the divs and takes charge of their functionality.

Here's what the functionality looks like:

animated gif showing accordion

Here's how I use it on a webpage:

CSS

.selected {
    opacity: 1.0;
}

.unselected{
    opacity: 0.4;
}

HTML

<div id="toAppend1"  style = " height: 800px; width: 100%; overflow:hidden; "></div>
<div id="toAppend2"  style = " height: 800px; width: 100%; overflow:hidden; "></div>

JavaScript to call

<script>
$(document).ready(function(){
    // note normally the content for the long and short pages would be different
    pageArray = ["t1.html", "t2.html", "t3.html", "t4.html"];
    longPageArray = ["t1.html", "t2.html", "t3.html", "t4.html"];
    addHorizontalAccordion('toAppend1', longPageArray, pageArray);
}
</script>

Here's the function that sets up the accordion.

// timing of animated transitions
const SHORT_TIME = 300;
const LONG_TIME = 2*SHORT_TIME;

// layout of accordion divs within parent divs
const PORTION_DIV = .85;
const MULTIPLE_TO = .65;
const HEIGHT_MULTIPLE_TO = .3;

// div aesthetics
const COLORS = [ "#ccf8ff", "ffe0b5", "#aad6a5", "#f0f8ff", "#d3d3d3",  "#fffefda", "#dfffcc"];
const BORDER_RADIUS = "0px";
const BORDER = "0px solid ";
const BORDER_STRING = "3px solid gray";

var width, height;
var widthPer, heightPer;

function addHorizontalAccordion(addName, longPageArray, pageArray)
{
    // creates a horizontal accordion of divs to load pageArray (collapsed div form) and longPayArray (expanded div form) 
    // all divs are added to div addName

    // first get the physical parameters of containing div
    width =  $('#'+addName).width()*.9;
    height = $('#'+addName).height();
    widthPer = width/pageArray.length;  // starting width when all divs are collapsed
    heightPer = height*HEIGHT_MULTIPLE_TO; 
    var widthPerSmall = Math.round((1-MULTIPLE_TO)*width/(pageArray.length - 1)); // collapsed width when one div is open       

    // create a div, set up its content and click functions for each remote page to load, as established by length of pageArray
    for(i = 1; i <= pageArray.length; i++){

    // div names indicate the containing div to which they are appended to avoid conflicts
    var divName = addName+"div_"+i
    $('#'+addName).append("<div id = '"+divName+"' class = 'unselected toAppend' style = 'padding: 20px; overflow:hidden; float:left;'></div>");

    $('#'+divName)
        .load(pageArray[i-1])
        .css({"width" : Math.round(widthPer*PORTION_DIV), "height" : heightPer, "background-color": COLORS[i-1], "cursor" : "pointer", "border-radius" : BORDER_RADIUS})
        .click(function(event){

        // must select target by name to ensure we are not using some child of desired div target
        var currentTarget = "#"+addName+"div_"+event.currentTarget.id.slice(-1);    
        if($(currentTarget).hasClass("unselected")){

            // every div other than currentTarget is added to shrinkString
            // currently selected div that should be shrunk to give way to currentTarget is divToShrink
            var shrinkString = ""
            var divToShrink = "";           
            for(ii = 1; ii <= pageArray.length; ii++){
                if(ii !== Number(event.currentTarget.id.slice(-1))){
                    var divShrink = "#"+addName+"div_"+ii;      
                    shrinkString = shrinkString + divShrink +", " ;
                    if($(divShrink).hasClass("selected")){
                        divToShrink = divShrink;
                     }            
                 }        
            }   

            // all divs with names in shrink string will be simultaneously shrunk
            shrinkString = shrinkString.slice(0, -2);
            //first shring previously enlarged div so there will be room to expand newly selected div
            $(shrinkString) 
            .animate({ width: widthPerSmall, height: heightPer}, LONG_TIME, function(){
                // animate change to larger size for selected div
                $(currentTarget)
                .animate({width: Math.round(MULTIPLE_TO*width), height: height}, LONG_TIME, function(){
                    // then load old content in now shrunken previously selected div
                    if(divToShrink.length > 1){
                    toShrink = divToShrink.slice(-1)
                    $(divToShrink)
                        .load(pageArray[toShrink-1])
                        .switchClass("selected", "unselected", SHORT_TIME)
                        .css({"background-color" : COLORS[toShrink-1], "border" : "0px solid black"});
                    }
                })
                .switchClass("unselected", "selected")
                .load(longPageArray[Number(event.currentTarget.id.slice(-1))-1 ]).attr("class", "selected")
                .css("background-color", "white").css("border", BORDER_STRING)
            })              

            // if there is no div already selected, just select div without having to unselect
            if(divToShrink.length < 1){
            // animate change to larger size for selected div
            $(currentTarget).animate({width: Math.round(MULTIPLE_TO*width), height: height}, LONG_TIME)
            // then switch to selected class, change style accordingly, and load larger content
                .switchClass("unselected" , "selected")
                .load(longPageArray[Number(event.currentTarget.id.slice(-1))-1 ])
                .css({"background-color" : "white", "border" : BORDER_STRING})
            }
        }
        })
    }
}

I welcome feedback on everything but especially on the following:

  1. Most importantly, this could certainly use some streamlining as performance is sluggish in the browser. Am I doing something wildly inefficent? What I know would guess is somewhat inefficient is shrinking all the divs that are not selected, even when many of them are already shrunk. Is it really so bad?
  2. I use some jQuery, but probably not enough. Are there any missed opportunities?
  3. This is clunky, ugly code. Should I refactor it to make it more OO? I kinda feel like it's not worth it, but are there some cool clean ups I'm missing?
  4. The function is enormous in length. I don't normally do this, but what are some non-artificial ways to break it up?
\$\endgroup\$
2
+50
\$\begingroup\$

Most importantly, this could certainly use some streamlining as performance is sluggish in the browser. Am I doing something wildly inefficent? What I know would guess is somewhat inefficient is shrinking all the divs that are not selected, even when many of them are already shrunk. Is it really so bad?

If you don't mind CSS3, you should take advantage of transitions. CSS animations are more optimized than methods like animate. Under the hood, animate is either requestAnimationFrame or timers (setInterval or setTimeout). Either way, they're slow because they're CPU-bound operations. CSS animations, on the other hand, are usually hardware-accelerated which means they take advantage of your GPU.

It also lessens your animate code since with just the transition property, the property to transition, and 2 or more style states declared in classes, CSS takes care of animating for you.

For instance, this example.

HTML:

<div class="page-wrapper"></div>

SCSS:

.page{
  display: inline-block;
  border: 1px solid #000;
  width: 100px;
  height: 150px;
  /* this property handles animation between .page and .page.open states */
  transition: width .3s, height .3s;

  &.open{
    width: 200px;
    height: 300px;
  }
}

JS:

$('.page-wrapper').on('click', '.page', function(){
  $(this).addClass('open').siblings().removeClass('open');
});

_.range(5).forEach(function(index){
  return $('<div/>', {class:'page'}).text(index).appendTo('.page-wrapper');
});

I use some jQuery, but probably not enough. Are there any missed opportunities?

See above. You can take advantage of SCSS for shorter CSS. You can use Lodash for some utility functions. range in conjunction with array iteration methods are good alternatives to loops. Also note the use of on which delegates handlers to a parent. That way I can attach page click handlers regardless if pages are there or not yet.

This is clunky, ugly code. Should I refactor it to make it more OO? I kinda feel like it's not worth it, but are there some cool clean ups I'm missing?

The function is enormous in length. I don't normally do this, but what are some non-artificial ways to break it up?

pageArray = ["t1.html", "t2.html", "t3.html", "t4.html"];
longPageArray = ["t1.html", "t2.html", "t3.html", "t4.html"];

Let's start with your variable names. pageArray makes a bit of sense, longPageArray... which is long? The pages or the array? The name isn't really helpful. Variable names should be self-explanatory and shouldn't rely on context to be understood.

        for(ii = 1; ii <= pageArray.length; ii++){
            if(ii !== Number(event.currentTarget.id.slice(-1))){
                var divShrink = "#"+addName+"div_"+ii;      
                shrinkString = shrinkString + divShrink +", " ;
                if($(divShrink).hasClass("selected")){
                    divToShrink = divShrink;
                 }            
             }        
        }   

There's this huge loop before your animations. This can cause a slight lag before animation, and here's why: It's a loop. JS is single-threaded, and if something takes too long, it will block everything including rendering. Another issue is that you appear to fetch an element inside the loop. DOM operations are slow. Best if you cache values, or avoid doing them in "hot" code.

.css({"width" : Math.round(widthPer*PORTION_DIV), "height" : heightPer, "background-color": COLORS[i-1], "cursor" : "pointer", "border-radius" : BORDER_RADIUS})

I also notice that you embed your CSS in your JS. I see that some are dynamic and calculated, but some are just static. You should put them in a CSS file. It's separation of concerns, and more importantly specificity. .css appends these styles as inline element styles. The only way you can override them from a stylesheet is using !important which you'd want to avoid, since it's the most powerful expression in CSS and nothing else can override it.

Your colors appear to go with the panes of the accordion, as well as pages, but they're in separate arrays. Why not merge them?

var pages = [{
  page: 'path/to/page',
  pageExtended: 'path/to/page/with/more/content',
  color: #FFFFFF;
},
// and so on
];

In related news, if these pages are really small, maybe you don't actually need separate HTML pages for each. You can simply write the HTML between <script> tags whose type is other thantext/javascript. The browser won't parse it, and you can use innerHTML to fetch the contents. This saves you network round-trips in fetching the pages.

<script type="not/javascript" id="page">
  <p>hello world</p>
</script>

var contents = document.getElementById('page').innerHTML; // <p>hello world</p>
\$\endgroup\$
  • \$\begingroup\$ Read your last part, and instead of storing the html pages within <script> tags, I would much rather store them in separate <div>'s, which could be hidden by default. \$\endgroup\$ – holroy Nov 13 '15 at 3:17

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.