JS Progress Bar Widget

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 = $("");
var html = "";
for(var step in this.steps){
html +=
"" +
""+(parseInt(step)+1)+"" +
"" +
"" +
""+this.steps[step]+"" +
"" +
"";
}
this.$node.html(html);
};
progressBar.prototype.goToStep = function(step){
console.log("got

Stuff I noticed (not in any strict order)

-
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.

-
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.

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

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

-
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 () { ... },
  // ...
};

-
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.

-
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.

-
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.

-
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 th

StackExchange Code Review Q#62981, answer score: 2