1
\$\begingroup\$

I'm still learning how to use jQuery. I created a function where when I click on some coordinate, it will then show a different image. I would like to make the code cleaner and more reusable, but don't know how.

https://jsfiddle.net/ewtt0kzg/

$('.main-image').click(function(e) {
  var main_image = $(this);
  var topleft = $(this).siblings('.single-page-image').eq(0).attr('src');
  var botLeft = $(this).siblings('.single-page-image').eq(1).attr('src');
  var botRight = $(this).siblings('.single-page-image').eq(2).attr('src');
  var topRight = $(this).siblings('.single-page-image').eq(3).attr('src');
  var width = $(this).width();
  var height = $(this).height();
  var offset = $(this).offset();
  var relativeX = (e.pageX - offset.left);
  var relativeY = (e.pageY - offset.top);
  if (relativeY < (height / 2) && relativeX < (width / 2)) {
    main_image.attr('src', topleft);
  }
  if (relativeY < (height / 2) && relativeX > (width / 2)) {
    main_image.attr('src', topRight);
  }
  if (relativeY > (height / 2) && relativeX < (width / 2)) {
    main_image.attr('src', botLeft);
  }
  if (relativeY > (height / 2) && relativeX > (width / 2)) {
    main_image.attr('src', botRight);
  }
});
.second {
  display: none;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<section class="single-page">
  <img class="single-page-image main-image" src="https://learn.getgrav.org/system/images/media/thumb-jpg.png" />

  <img class="single-page-image second top-left" src="https://media.giphy.com/media/1bBMTxUs4iRTG/giphy.gif" />

  <img class="single-page-image second bottom-left" src="https://webkit.org/demos/srcset/image-src.png" />

  <img class="single-page-image second top-right" src="http://hubblesite.org/home-page/graphics/middle_container/Homepage-announce-gallery-400x400.png" />

  <img class="single-page-image second top-left" src="http://static1.squarespace.com/static/560aa49ee4b0b632352fe46f/t/560d517de4b08ac155043394/1457617167699/" />
</section>

\$\endgroup\$
  • \$\begingroup\$ What is the intended usage? Do the images represent anything? Should the user know what the expected behaviour is when various parts of the image are clicked? \$\endgroup\$ – 200_success Oct 4 '17 at 17:16
4
\$\begingroup\$

Overly complex

The code you have is overyly complex

Some issues

  • Don't use classes to locate single items.

If you wish to identify single elements use the id property to create a unique name and use that to locate and element.

  • Don't define what is not needed.

At the beginning of your function you define each image's src but the function will only ever use one of them. Rather than define them all set the src in the individual condition blocks. The same with var main_image = $(this); there is no reason you need to do this, just use the reference $(this) or better yet this. You don't have to wrap it in jQuery's code if not needed.

  • Don't mix and match naming styles.

You name a variable using snake_case in one spot then in others you use camelCase. Not that it affects your code, but later when you need to recall a variable, you remember it by its name, if you mix and match, can you be sure you remember if it is snake_case or camelCase.

Use the one style (camelCase) as that is what JavaScript uses and you will never have to guess, or look up what style a variable used.

  • Use else

If you have a set of mutually exclusive conditional statement you should use else. Only one of the conditions will pass so there is no need to test other conditions if one has passed. Also as at least one condition has to pass the last else need not have the if statement it is the last and only option left so it must pass.

  • Use constants if a variable will not change.

var foo = "data" the variable foo can be assigned a new value which is all good if that is the intention. But many time you assign a variable a value only once. If you at some stage accidentally assign that variable a value due to a typo in the source code, you may spend hours, days even looking for the problem, as where the problems occur may not be where the problem starts. And worse, you may not spot the problem due to lack of testing until your code is out there.

const foo = "data" This tells Javascript that the variable foo is a constant and will never change. If you try to change it (via typo in source code) Javascript will throw an error, letting you detect the problem and direct you to the location of the problem. You can fix it there and then saving a lot of time and effort.

Though it is not a major point in this example, using the correct variable type in every situation helps build good habits.

  • Don't repeat

Modern languages came from a need to keep lazy programmers happy. There is nothing worse than having to repeat the same code over and over. For you typing it, it means you are more likely to make a mistake, it means it takes longer to get a result, and it means (mostly but not always) your code will take longer to run. The worst bit about repeating code is when you have to change it.

You have the variables width and height and for each test you do the same thing offsetX < width / 2, repeating the same code.

Do it once const halfWidth = $(this).width() / 2; event better do the width test only once const leftClicked = (e.pageX - offset.left) < ($(this).width() / 2);

Rewrite of your code.

// Note for lines assigning leftClicked, rightClicked
// I use (...) to wrap expressions though not needed,
// it is good habit to do so for readability
$(".main-image").click(function(e) {
    const images = $(this).siblings(".single-page-image");
    const offset = $(this).offset();
    const leftClicked = (e.pageX - offset.left) < ($(this).width() / 2);
    const topClicked = (e.pageY - offset.top) < ($(this).height() / 2);  
    var imageIndex = 2; // default image index for bottom right
    if (topClicked){
        if (leftClicked) {
            imageIndex = 0;
        } else {
            imageIndex = 3;
        }
    } else if (leftClicked) {
        imageIndex = 1;
    }
    $(this).attr("src", images.eq(imageIndex).attr("src"));
});

Is there a better way?

It is often better to embed as much context / semantic specific information in the page.

In this example you could have used a map element to define the click areas and used Javascript just to change the image src.

Example

The example shows it done with minimum required code. I know that the sandboxed page is completely under my control and can thus reliably use directly referenced elements to add functionality. It is done without jQuery to keep it simple.

imageMap.addEventListener("click", (e) => {
  mainImage.src = window[e.target.dataset.imageId].src;
});
.hide { display: none; }
<map id="imageMap" name="imageAreas">
  <area shape="rectangle" coords="0,0,200,200" title="One" data-image-id = "image1"/>
  <area shape="rectangle" coords="200,0,400,200" title="Two"  data-image-id = "image2"/>
  <area shape="rectangle" coords="0,200,200,400" title="Three"  data-image-id = "image3"/>
  <area shape="rectangle" coords="200,200,400,400" title="Four"  data-image-id = "image4"/>
</map>

<section>
  <img id="mainImage" usemap="imageAreas"  src="https://learn.getgrav.org/system/images/media/thumb-jpg.png" />
  <img id="image1" class="hide" src="https://media.giphy.com/media/1bBMTxUs4iRTG/giphy.gif" />
  <img id="image2" class="hide" src="https://webkit.org/demos/srcset/image-src.png" />
  <img id="image3" class="hide" src="http://hubblesite.org/home-page/graphics/middle_container/Homepage-announce-gallery-400x400.png" />
  <img id="image4" class="hide" src="http://static1.squarespace.com/static/560aa49ee4b0b632352fe46f/t/560d517de4b08ac155043394/1457617167699/" />
</section>

A word about jQuery

A word of warning. JQuery is a popular library, and in its day was a good solution to the problem of cross browser compatibility, and has some relevance today (dam IE11). Unfortunately it has produced a generation of front end coders that are unable to use the DOM directly. As a beginner you are training your self for the future, jQuery is struggling to stay relevant now. Don`t invest precious time where you don't need to. The step from vanilla to JQuery is a lot easier than the other way around, so be careful where you invest your time.

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