5
\$\begingroup\$

I would like some comments on my code and implementation for this simple piece of jQuery.

Basically it is 2 div boxes #side1 and #side2, in a container div called #frontimage, where #side2 can be swung out by dragging from right to left along #frontimage.

My jQuery code below continuously updates the CSS transform rotateY parameters for #side2 based on a mousemove event.

Here is a working example:

http://jsfiddle.net/Sr4zw/embedded/result/

And here is the complete code including HTML and CSS:

http://jsfiddle.net/Sr4zw/

$(document).ready(function() {

    var yDegrees = 0, slidingLength = 372, maxRotation = -45;

    $("#frontimage").on('mousedown', function( event ) {

        var originX = event.pageX;

        $("#frontimage").on("mousemove", function( event ) {

            yDegrees = yDegrees + (((originX - event.pageX) / slidingLength) * maxRotation);
            if (yDegrees < maxRotation) {
                yDegrees = maxRotation;
            };
            if (yDegrees > 0) {
                yDegrees = 0;
                };

            originX = event.pageX;

            $("#side2").css({
                '-webkit-transform': 'rotateY(' + yDegrees + 'deg)',
                '-moz-transform': 'rotateY(' + yDegrees + 'deg)',
                '-ms-transform': 'rotateY(' + yDegrees + 'deg)',
                '-o-transform': 'rotateY(' + yDegrees + 'deg)',
                'transform': 'rotateY(' + yDegrees + 'deg)'
            });
        });
    });


    $('html').on('mouseup', function() {

        $("#frontimage").off('mousemove');

    });
});

Some comments on my implementation:

yDegrees stores the current rotation degree. slidingLength is the length (in pixels) of the mouse drag that changes yDegrees.

\$\endgroup\$
  • 1
    \$\begingroup\$ As per the FAQ, please embed the code you'd like reviewed. \$\endgroup\$ – Jamal Nov 18 '13 at 17:39
  • 1
    \$\begingroup\$ Right, I've done that now. \$\endgroup\$ – athis Nov 18 '13 at 17:44
  • \$\begingroup\$ @Malachi: Any ideas on a better title? \$\endgroup\$ – Jamal Nov 18 '13 at 17:53
  • \$\begingroup\$ @Jamal, working on it. not sure what the thing does yet. but I do see some code that looks wrong. I am no good at figuring out Titles.....lol \$\endgroup\$ – Malachi Nov 18 '13 at 17:54
  • \$\begingroup\$ The code manipulates the CSS transform properties of a div by dragging left and right on the div. I'm updating the title. \$\endgroup\$ – athis Nov 18 '13 at 18:05
3
\$\begingroup\$

Edit: Something else that I now noticed while I was reviewing this answer, your if statements could be written differently, it looks like it should be an if/else statement rather than two separate if statements.

We know that if yDegrees is less than maxRotation that it is not equal to zero, nor will it be equal to zero after yDegrees is set to -45(maxRotation).

Here is your code

    if (yDegrees < maxRotation) {
        yDegrees = maxRotation;
    };
    if (yDegrees > 0) {
        yDegrees = 0;
        };

and here is what I am proposing

if (yDegrees < maxRotation) {
    yDegrees = maxRotation;
} else if (yDegrees > 0) {
    yDegrees = 0;
}

If you wanted to, you could make this a one line piece of code in your JavaScript by writing a ternary inside of a ternary like this:

yDegrees = yDegrees < maxRotation ? maxRotation : (yDegrees > 0 ? 0 : yDegrees);

I also noticed that you set originX again after these if statements, but don't actually change it's value anywhere since the last time that you set it, and you don't use it again after this second setting of the variable, I assume that it is dead code, you should just get rid of that extra variable set.


From one Beginner to another,

the code looks good to me,

  1. The formatting is clean
  2. The code flows well and it isn't extremely hard to follow the code.
\$\endgroup\$
6
\$\begingroup\$

The semicolons after the closing braces are really unnecessary, and the indentation is a bit off here:

        if (yDegrees < maxRotation) {
            yDegrees = maxRotation;
        };
        if (yDegrees > 0) {
            yDegrees = 0;
            };

It would be better this way:

        if (yDegrees < maxRotation) {
            yDegrees = maxRotation;
        }
        if (yDegrees > 0) {
            yDegrees = 0;
        }

This could be written more compactly:

yDegrees = yDegrees + (((originX - event.pageX) / slidingLength) * maxRotation);

Like this:

yDegrees += (originX - event.pageX) / slidingLength * maxRotation;

That is, using += and removing unnecessary parentheses.

\$\endgroup\$

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.