2
\$\begingroup\$

I created my first plugin in jQuery and just want some inputs on how I can improve the code and features.

Here is the fiddle

    $.fn.fruitZoom = function (options) {

var defaults = {
    ele: "p,span",
};

var settings = $.extend(defaults, options);
$("#fruitZoom :first-child").click(function () {

    fontSize = $(defaults.ele).css("font-size")
    $(defaults.ele).css("font-size", parseInt(fontSize) + 2);
    console.log(fontSize)
});
$("#fruitZoom :last-child").click(function () {
    fontSize = $(defaults.ele).css("font-size")
    $(defaults.ele).css("font-size", parseInt(fontSize) - 2);
    console.log(fontSize)
});
};
})(jQuery);
options = {
    ele: "p,h1",
}
$("#fruitZoom").fruitZoom(options);
\$\endgroup\$
2
\$\begingroup\$

Flexibility

Your plugin relies on very specific markup; an element with an ID of fruitZoom having a first and last child element meant to be used as zoom in and zoom out buttons, respectively.

This is not flexible enough to be generally useful. What if you want multiple zoom controls on the page at once? What if you want to use a slider instead of buttons, or keyboard controls?

Naming and style

The identifier ele is not appropriate for a user-facing option name. The name elements would be an improvement. Better yet, use something more descriptive of what the value should to be, such as selector. After all, the user does not assign elements to this property, they assign a selector to it.

Your use of semicolons is inconsistent. Don't rely on automatic semicolon insertion in some places and not in others. Either terminate every statement with a semicolon, or only use them as separators in places where they are necessary. Pick a style and stick to it.

Behavior

This line here...

fontSize = $(defaults.ele).css("font-size")

...only gets the size of the first element matched by the ele selector. That means as soon as you zoom in or out, all elements matching your selector are resized to the size of the first element. You can see this in your own example code on JSFiddle; as soon as you zoom, the heading changes its size to match the first paragraph. This is probably not desirable behavior.

Redundancy

This block of code...

$("#fruitZoom :first-child").click(function () {
    fontSize = $(defaults.ele).css("font-size")
    $(defaults.ele).css("font-size", parseInt(fontSize) + 2);
    console.log(fontSize)
});

...is nearly identical to the next:

$("#fruitZoom :last-child").click(function () {
    fontSize = $(defaults.ele).css("font-size")
    $(defaults.ele).css("font-size", parseInt(fontSize) - 2);
    console.log(fontSize)
});

This is a sure sign that you should refactor your code to eliminate redundancy. Consider writing a named function, replacing the + 2 / - 2 part with + amount, where amount is a variable you pass in, having a value of 2 or negative 2.

\$\endgroup\$
  • \$\begingroup\$ I tried to fix few issues but unable to remove redundancy part. link \$\endgroup\$ – fruitjs Jul 11 '14 at 10:59
  • \$\begingroup\$ @user3534656 did you see my comments on your post? They remove the redundancy. Also, .css has a special syntax for incrementing/decrementing values. If you use that, it fixes the problem of everything getting scaled to the same size. jsfiddle.net/zpYb4/6 \$\endgroup\$ – Dagg Jul 11 '14 at 11:08

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.