5
\$\begingroup\$

Demo of the widget: http://jsfiddle.net/slicedtoad/Lywvbsf4/

It's a progress bar that shows a list of steps and which one is being completed as well as allowing previous steps to be revisited.

It will be used to display the user's form completion progress on a web app that favors a client side approach (forms are submitted to the server at the last step).

I'd like feedback specifically on the widget based approached I took. I had the goals:

  • simple interface
  • low coupling to the rest of the app
  • easy to extend the functionality (it should be easy to add more callbacks to handle new functionality, for example)
  • no code bloat from
    • legacy support (the app is only supported on FF and Chrome)
    • unnecessarily portability and safety (i.e. it needs to be safe in this app, not every app)

I ended up making a lot of fairly arbitrary decisions, which I would like critiqued.

Widget:

var progressBar = function(id,options){
    this.init(id,options);
};
progressBar.prototype.init = function(options){
    this.id = options.id; // ID of the div that will store the progress bar
    this.$node;
    this.steps = options.steps;
    this.current = 0; // current step
    this.previousClickedCallback = options.previousClicked;
    this.draw(); // populate $node
    $(this.id).append(this.$node); // append $node to the DOM
    this.$node.on("click",".step-finished .step-number",
                  $.proxy(this.previousClick,this));
    this.next();
};
progressBar.prototype.previousClick = function(e){
    this.previousClickedCallback($(e.target).html());
};
progressBar.prototype.draw = function(){
    this.$node = $("<ol class='progressbar container'></ol>");
    var html = "";
    for(var step in this.steps){
        html +=
            "<li class='step'>" +
                "<div class='step-number'>"+(parseInt(step)+1)+"</div>" +
                "<div class='step-line'></div>" +
                "<div class='step-label-wrap'>" +
                    "<label class='step-label'>"+this.steps[step]+"</label>" +
                "</div>" +
            "</li>";
    }
    this.$node.html(html);
};
progressBar.prototype.goToStep = function(step){
    console.log("gotostep: ",step);
    if(step>=1 && step<=this.steps.length+1){
        this.current = parseInt(step);

        // Reset progress bar status
        this.$node.find(".step-current").removeClass("step-current");
        this.$node.find(".step-finished").removeClass("step-finished");

        // Set current step
        this.$node.find(".step:nth-child("+this.current+")").addClass("step-current");
        // Set all previous steps to finished
        for(var i = step-1; i > 0; i--){
            this.$node.find(".step:nth-child("+i+")").addClass("step-finished");
        }
    }
};
progressBar.prototype.next = function(){
    if(this.current<this.steps.length+1){
        this.goToStep(this.current+1);
    }
};
progressBar.prototype.previous = function(){
    if(this.current>=2){
        this.goToStep(this.current-1);
    }
};

Example usage:

var goBack = function(step){
    //cache current form or whatever
    pbar.goToStep(step);
};

var pbar = new progressBar({
    id:"#ProgressBar",
    steps:["Date","Items","Preview","Details","Confirm"],
    previousClicked:function(step){goBack(step);}
});
$("#next").on("click",function(){pbar.next()});
$("#previous").on("click",function(){pbar.previous()});

CSS (minimal):

ol.progressbar {
    list-style-type: none;
    padding-left:0;
}
.progressbar.container {
    display: flex;
    align-items: flex-end;
    padding-top: 22px;
}
.progressbar .step {
    flex-grow:1;
    position:relative;
    text-align: center;
    z-index:1;
}
.progressbar .step-number {
    position:relative;
    z-index:10;
    width: 20px;
    height: 20px;
    border-radius: 10px;
    background-color:white;
    display:inline-block;
    text-align: center;
    line-height: 20px;
    border:1px solid;
}
.progressbar .step-finished .step-number:hover{
    background-color: lightblue;
    cursor: pointer;
}
.progressbar .step-finished .step-number{
    background-color: green;
}
.progressbar .step-current .step-number{
    background-color:lightblue;
}
.progressbar .step-label-wrap {
    position: absolute;
    width:100%;
    top: -22px;
}
.progressbar .step-label {
    padding: 0px 1px;
}
.progressbar .step-line {
    height: 2px;
    background-color: black;
    width:100%;
    position:absolute;
    top: 50%;
    left:50%;
    z-index:5;
}
.progressbar .step:last-of-type .step-line{
    display:none;
}
\$\endgroup\$
  • \$\begingroup\$ why isn't my JS highlighting properly? \$\endgroup\$ – slicedtoad Sep 15 '14 at 21:02
  • 2
    \$\begingroup\$ fixed - the highlighter looks at tags, and wasn't sure if the blocks were JS or CSS (it seemed to settle on assuming everything was CSS). You can add hints to avoid this \$\endgroup\$ – Flambino Sep 15 '14 at 21:28
  • \$\begingroup\$ oh, it's lang-js. I tried to add javascript as the language hint. \$\endgroup\$ – slicedtoad Sep 16 '14 at 13:32
2
\$\begingroup\$

Stuff I noticed (not in any strict order)

  1. This is not what I expected, given the name "progress bar". Yes, the name makes perfect sense word-for-word, but "progress bar" usually means something very specific - and different. Even non-coders, I think, wouldn't call this a progress bar, because progress bars are those things that just fill up (in one direction only) over time. I'd perhaps call this a "checklist" instead, since it has discrete steps.

  2. This would probably work well as a jQuery plugin, which would also be the more conventional approach. jQuery plugins also have a whole list of conventional patterns for setting options and such, which would make a lot of sense here. Use such conventions to your advantage; saves you the trouble of reinventing it all.

  3. Constructor functions should be PascalCase, i.e. ProgressBar - not progressBar.

  4. Why not do initialization in the constructor? It's pretty much its job, yet you delegate that to an init function.

  5. You can clean up you code by defining the prototype object wholesale, instead of prefixing everything with constructor.prototype.:

    function ProgressBar() {
      // ...
    }
    
    ProgressBar.prototype = {
      next: function () { ... },
      previous: function () { ... },
      // ...
    };
    
  6. Warning, side-effects and mixed responsibilities:

    this.draw(); // populate $node
    $(this.id).append(this.$node); // append $node to the DOM
    

    So draw builds an element hierarchy in memory. But it doesn't return it. It assigns it to a variable, which some other code then has to go grab it from. But that only makes sense if draw has for sure been called. And called only once! Otherwise things break.
    Point is, it's a tangled mess. A better approach would be to have draw be in charge of inserting the elements, or make draw simply return the elements it creates like a factory function. Right now, it's neither here nor there. Personally, I'd pick the latter, call the function build rather than "draw", and not make it a prototype method, but an internal private function in the constructor.

  7. Then again, I'd much rather define my checklist in the HTML as a regular ol element, and simply use the JS to add the behavior (not the content or structure) for that list. Right now, you have HTML in you JavaScript, when those things should be kept separate. Structure in markup, style in CSS, and behavior in JavaScript. Yes, the lines do of course blur a lot, and many widgets do inject large chunks of HTML, but I'm wary of those approaches. One might ask why it doesn't inject all the CSS too, while it's at it.
    I'm not saying you should throw it all out, but do consider separating things.

  8. Don't leave console.log statements in production code. At least not without safety checks/polyfills to ensure that console.log actually exists and is a function. Sure you only targeting a few browsers, but I'll get into that later.

  9. Why is there only a callback option for going back? Why not use events instead, and fire them off for any state change? jQuery provides a nice, simple custom event API you can use. If you want callbacks instead of events, then I'd say add them now. You say it's easy to add such things, so why not? You may find it's not as easy as you think, or that a different interface might be even better.
    Besides, there's something weird going on with previousClick being a function that gets called only by external code - but which then invokes a callback, that's also defined externally. Why is the ProgressBar involved in that at all? And why does it pass a bunch of HTML as a string to the callback? Also, it'll fail immediately, if you call previousClick but haven't passed in a callback when instantiating the object.

Outside the code, this caught my eye.

  • low coupling to the rest of the app
  • [...]
  • no code bloat from
    • legacy support (the app is only supported on FF and Chrome)
    • unnecessarily [sic] portability and safety (i.e. it needs to be safe in this app, not every app)

So... your code is actually very coupled to the app after all. It may not be coupled though individual parts of the code, but the result is the same: You're prevented from easily using the widget elsewhere, and maintainability may be a hassle (if, say, the rest of the app gets updated to run on more browsers, you have that much more code to test/update). Pretty much the same things that'd apply if the code was tightly coupled to the app. Portability is basically decoupling on a larger scale.

Some of your decisions, like only having a callback for going backwards also betray coupling. Coupling in the sense that if you didn't need it for this particular app, in this particular context, you just didn't add it. Again, that's fair, but don't then say it's got low coupling.

Specifically because you aimed for a self-contained little widget that should be all the more reason to also aim for compatibility and portability. Sure, there's the usual cost/benefit weighing you'll have to do, and maybe it really doesn't make sense to support more browsers. However, the smaller and the mode self-contained the project, the easier it is to ensure good browser support. In fact, I'd wager the JS will run just fine on most browsers as-is; any issues will likely all be CSS-related. If I were you, I'd at least have tested it in other browsers, just out of curiosity.

At any rate, don't trick yourself into thinking it's decoupled, because in a sense it really, really isn't.

\$\endgroup\$
  • \$\begingroup\$ Quick note for #8, previousClick is called when the click event fires on a "finished" step (green circle). The idea was to allow the app to handle what happens when a user tries to go back to previous step (this could be complex or not allowed in some cases). It's name makes it confusing since I have a button called "Previous", but previousClick has nothing to do with it. \$\endgroup\$ – slicedtoad Sep 16 '14 at 13:24
  • \$\begingroup\$ btw, thanks for the feedback. I especially like #6's suggestion of having the widget modify an existing html list. Feels a lot cleaner. \$\endgroup\$ – slicedtoad Sep 16 '14 at 13:28
  • \$\begingroup\$ @slicedtoad The user going back is one thing, but he/she might also try skipping ahead. Both represent state changes, so it'd be nice to handle either one in a generalized way. You widget is basically a visual state machine, so perhaps you can find some inspiration from those things. Here's one I reviewed recently. In your case, you could also look into using pushState/popState to support the browser's own back/forward buttons. \$\endgroup\$ – Flambino Sep 16 '14 at 20:41
  • \$\begingroup\$ Alright, thanks, I've made some major changes and put a follow up question here: codereview.stackexchange.com/questions/63193/… \$\endgroup\$ – slicedtoad Sep 17 '14 at 20:27

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.