jQuery .each() for repeating elements

Question

I'm trying to use jQuery to detect the orientation of a number of images. Once detected, I want to add a class to indicate this i.e .landscape.

The code below seems to only be assessing the first image it comes across, and then applying that to all subsequent images. For example, if the first image is landscape, this will be applied to all images including portrait or square image.

Am I misusing the .each() statement?

Thank you.

HTML

<div class="carousel-cell">
    <img src="domain.com" data-width="100" data-height="200" />
</div>
<div class="carousel-cell">
    <img src="domain.com" data-width="200" data-height="100" />
</div>
<div class="carousel-cell">
    <img src="domain.com" data-width="100" data-height="100" />
</div>

JQUERY

// Detect image orientation 
$('.carousel-cell').each(function () {
"use strict";

var img = $('img');

if (img.attr('data-width') > img.attr('data-height')){
    // Landscape
    img.removeClass().addClass('landscape');
} else if (img.attr('data-width') < img.attr('data-height')){
    // Portrait
    img.removeClass().addClass('portrait');
} else {
    // Equal (Square)
    img.removeClass().addClass('square');
}
});

`$("img")` will select every `` on the page. If you want to refer to the *current* img in the loop, use `$(this).find("img")` instead. — Tyler Roper, Oct 02 '18 at 14:32
replace this code and try -> var img = $(this).children('img'); — alireza khosravi, Oct 02 '18 at 14:36

Tyler Roper · Accepted Answer · 2018-10-02T14:54:09.250

1

In each loop, you're selecting $("img"), which is every <img> on the page.

You need to limit your search to find the img within the current element, using $(this).find("img") instead.

// Detect image orientation 
$('.carousel-cell').each(function() {
  "use strict";

  var $img = $(this).find('img');
  var w = parseInt($img.attr("data-width"));
  var h = parseInt($img.attr("data-height"));
  $img.removeClass();

  if (w > h) {
    $img.addClass('landscape');
  } else if (w < h) {
    $img.addClass('portrait');
  } else {
    $img.addClass('square');
  }
});

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

<span><strong>Right click on image > Inspect Element</strong> to see classes.</span>

<div class="carousel-cell">
  <img src="domain.com" data-width="100" data-height="200" />
</div>
<div class="carousel-cell">
  <img src="domain.com" data-width="200" data-height="100" />
</div>
<div class="carousel-cell">
  <img src="domain.com" data-width="100" data-height="100" />
</div>

EDIT: Per your comment about the weird behavior, I suspect it's simply because you're not parsing the width and height as integers, and as such, the > and < comparisons will be done alphabetically. For example, "1000" would be less than "800", because "1" < "8". I've added parseInt() to resolve this.

edited Oct 02 '18 at 14:54

answered Oct 02 '18 at 14:40

Tyler Roper

21,445
6
33
56

Thank you for the answer, this makes total sense. However, very oddly I am getting inconsistent results, where some img that should be portrait are still being treated as landscape. Any ideas? Most are now correct but I have one listed as data-width='854' data-height='1280' and it is still being given a class of landscape? – dungey_140 Oct 02 '18 at 14:44
Hmm. You probably need to parse the images widths and heights as integers, as attribute values will default to strings. I'll edit my answer. – Tyler Roper Oct 02 '18 at 14:45
@dungey_140 I've updated my answer, please check to see if that resolves your funky behavior. – Tyler Roper Oct 02 '18 at 14:48
Legend, this works perfectly. Thanks so much for taking the time to explain and resolve the issue, despite it going beyond the OQ. Many thanks, Pete – dungey_140 Oct 02 '18 at 14:52
@dungey_140 if you are just doing stuff to the image, why not just loop the images, rather than the cell containing them? – Pete Oct 02 '18 at 14:54
you can also write it a little shorter like `$("img", this)` – Velimir Tchatchevsky Oct 02 '18 at 14:55
@Pete has a point. If you wanted to go that route (it's likely more efficient), change the original `.each` selector to be `.carousel-cell > img`. The `var $img` would then just be `$(this)`, without the need for a chained `.find()`. – Tyler Roper Oct 02 '18 at 14:56

Hary · Answer 2 · 2018-10-02T14:45:20.610

You should use $(this) and then .find image tag inside that.

$(function(){

// Detect image orientation 
    $('.carousel-cell').each(function () {
    "use strict";
    
    var img = $(this).find("img");
    var imgWidth = $(img).attr('data-width');
    var imgHeight = $(img).attr('data-height');
    
    var className = imgWidth > imgHeight 
                    ? "landscape" 
                    : imgWidth < imgHeight
                      ? "portrait"
                      : "square"; 

     $(img).removeClass().addClass(className);
     
    });

});

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div class="carousel-cell">
    <img src="domain.com" data-width="100" data-height="200" />
</div>
<div class="carousel-cell">
    <img src="domain.com" data-width="200" data-height="100" />
</div>
<div class="carousel-cell">
    <img src="domain.com" data-width="100" data-height="100" />
</div>

*"Also, avoid using nested"* - OP did not use any nested logic, just a simple `if, else if, else`. Your example actually *added* a nested ternary. — Tyler Roper, Oct 02 '18 at 14:44

jQuery .each() for repeating elements

2 Answers2