Simplify Raphael JS code: display text in DIV on click event

Question

I've been working with the RaphaelJS 'australia map' example that displays text in a DIV when a state is clicked on. However I'm having difficulty working with it to create different events and functions.

I want to be able to duplicate the 'display text' function for two different element onClick events (i.e. have two different buttons (elements) that do the same thing). But the current code is a bit too complicated for me. Can somebody please help me simplify the code that displays the text in a DIV when an element is clicked on.

Any help would be much appreciated - this is what I'm currently working with jsfiddle.

 window.onload = function () {
 var R = Raphael("paper", 532, 500);
 var attr = {
 fill: "#0B0",
 stroke: "#fff",
 "stroke-width": 3,
 };
 var ctx = {};
 ctx.btn_1 = R.path("M 180,130 L 340,130, L 392,212, L 128,212 z").attr(attr);
 ctx.btn_2 = R.path("M 128,212 L 392,212, L 435,280, L 85,280 z").attr(attr);
 var current = null;
 for (var state in ctx) {
 (function (st, state) {
 st[0].style.cursor = "pointer";
 st[0].onmousedown = function () {
 current && (document.getElementById(current).style.display = "");
 st.toFront();
 R.safari();
 document.getElementById(state).style.display = "block";
 current = state;
 };
 })(ctx[state], state);
 }
 };

• Kevin Nielsen: That is actually reasonably compact, straightforward code. Which part were you hoping to simplify, exactly? One thought: if the click handler is what you're focusing on, you can always declare the function independently and assign it by variable name, rather than reproducing it inline repeatedly.
• user1619342: The part of code I'm hoping to simplify is the click handler (starting from var current = null; to the end of the code). The problem I'm having is trying to rebuild the click handler code - or even calling the function to be used elsewhere.
• Dan Lee: You could user Paper.set and make an array of elements. I wouldn't add it to the context as class property. And btw, you can add 'cursor': 'pointer' to your attr. And at last: Don't use for..in, because it's very vulnerable, try Google :)
• user1619342: Great, thanks Dan Lee. I'll look into those more. Google to the rescue! :)

Answer 1

I agree with tiffon it's better to extract a function, but I don't agree with the rest of their edits, so here's what I would have done (I also kept the style intact).

var initPath = (function () {
 var currentEl;
 function init(el, path) {
 path.attr({
 fill: "#0B0",
 stroke: "#fff",
 "stroke-width": 3,
 });
 path[0].style.cursor = "pointer";
 path[0].onmousedown = function () {
 currentEl && (currentEl.style.display = "");
 path.toFront();
 path.paper.safari();
 el.style.display = "block";
 currentEl = el;
 };
 }
 return init;
})();
window.onload = function () {
 var R = Raphael("paper", 532, 500);
 var paths = {
 'btn_1': R.path("M 180,130 L 340,130, L 392,212, L 128,212 z"),
 'btn_2': R.path("M 128,212 L 392,212, L 435,280, L 85,280 z")
 };
 for (var id in paths) {
 initPath(document.getElementById(id), paths[id]);
 }
};

Firstly, I think st and state were confusing names, so I changed them to be path and id, respectively, and then changed id to el to avoid additional lookups.

I separated the infrastructure (current and initPath) from the usage (R, paths and the loop).

I also think an object literal looks better than a series of assignments, so I changed that.

Finally, I like it better if I move initPath and currentEl into a closure of their own—after all, they don't depend on window.onload anymore.

I left the inner init a named function so it is named in a stack trace, if there is an error.

Answer 2

I gave it a shot.

window.onload = function () {
 var R = Raphael("paper", 532, 500),
 attrs = {
 fill: "#0B0",
 stroke: "#fff",
 "stroke-width": 3,
 cursor: "pointer"
 },
 paths = [
 {id: 'btn_1', p:"M 180,130 L 340,130, L 392,212, L 128,212 z"},
 {id: 'btn_2', p:"M 128,212 L 392,212, L 435,280, L 85,280 z"}
 ],
 current,
 max,
 i;
 for (i = 0, max = paths.length; i < max; i++) {
 initPath(paths[i].p, attrs, paths[i].id);
 }
 function initPath(pathStr, attrs, targetId) {
 var path = R.path(pathStr).attr(attrs),
 txElm = document.getElementById(targetId);
 path.click(function (e) {
 if (current) {
 if (current === txElm) {
 return;
 }
 current.style.display = '';
 }
 current = txElm;
 current.style.display = 'block';
 this.toFront();
 R.safari();
 });
 }
 }​

Integrated Dan Lee's comments and defined a function instead of making a new closure in each loop iteration.

• Dan: I see absolutely no problem with using for... in to enumerate object properties, Google did not help. The original click logic was idiomatic JavaScript, not sure why you changed it. Also, OP doesn't use jQuery ([0] comes from Raphaël).