Breaking group of items into rows then ensure consistent height of each row element

Question

I'm working on a product category page where the product images have inconsistent heights so I've been asked to improve the page appearance without changing the images.

The page contains x number of products in a 3 column grid.

I decided to set the height of the image wrapper to the height of the image with the maximum height in each row, then use flex to vertically center the images.

How could I improve my JavaScript?

/**
 *  Ensure all rows appear the same height even if they contain products with
 *  small images.
 */

const imgWrappers = Array.from(document.querySelectorAll('.summary-image-wrapper'));
const rows = [];

// 3 Products per row - First group the image wrappers into groups of 3
while (imgWrappers.length !== 0) {
    rows.push({
        elements: imgWrappers.splice(0, 3),
    });
}

// Then establish which element is the tallest in it's group
rows.forEach((row) => {
    let height = 0;

    row.elements.forEach((el) => {
        const currHeight = el.firstElementChild.getAttribute('height');
        height = currHeight > height ? currHeight : height;
    });

    row.height = height;

    // Set the height of each element to that of the tallest element in the row
    row.elements.forEach((el) => {
        el.style.height = `${row.height}px`;
    });
});

<div class="container">
    <div class="row">
        <div class="col-6 col-md-4">
	        <div class="summary-product">
	            <a href="http://examplelink.com/" class="woocommerce-LoopProduct-link woocommerce-loop-product__link">
                    <div class="summary-image-wrapper" style="height: 413px;">
                        <img width="300" height="413" src="http://via.placeholder.com/300x413" class="attachment-woocommerce_thumbnail size-woocommerce_thumbnail wp-post-image" alt="alt ttext" srcset="http://via.placeholder.com/300x413 300w, http://via.placeholder.com/218x300 218w, http://via.placeholder.com/350x482 350w, http://via.placeholder.com/581x800 581w" sizes="(max-width: 300px) 100vw, 300px">
                    </div>
                    <h2 class="woocommerce-loop-product__title summary-product-title">Example title</h2>
	                <span class="price summary-product-price">POA</span>
                </a>
	        </div>
            <!-- Further .summary-products have been omitted for brevity--> 
        </div>
     </div>
</div>

EDIT: I discovered an error within my original code due to string comparison. Take the following row of 3 images and their height:

Image a: 365

Image b: 91

Image c: 401

Given the above images using the code originally given 'image b' will be considered the largest. This is because el.firstElementChild.getAttribute('height') returns a string, when comparing strings javascript does it "alphabetically". This means that js looks at the first character of the string then the second and so on. When comparing two numberic strings if a string starts with a 9 it will always be the largest!

'10000000000' > '9' //false
'89999999999' > '9' //false
'91' > '9' //true

To ensure the original code works the value stored in currHeight needs to be converted to a number e.g currHeight = Number.parseInt(el.firstElementChild.getAttribute('height')); before comparison.

Answer 1

Keep it simple

Your algorithm is too complex, with too many iterations and uses too much memory. It can be simplified using only a single outer iteration to get a row, and the row's max height. Then an inner loop to set the new height values. (see rewrite)

Some points

• document.querySelectorAll returns an iterable object. There is no need to convert it to an array. You can use for of or any of the iteration features on it. EG document.querySelectorAll("query string").forEach( and [...document.querySelectorAll("query string")]

• You only need to use .getAttribute when the attribute is not part of the DOM. const height = element.height is more concise.

• You can use the Math function max to get the maximum of 2 or more values at a time. maxHeight = Math.max(maxHeight, element.height)

• Don't pollute the global scope. Use a self invoking function to encapsulate your variables. ;(()=>{ /* your code in here */ })();

Rewrite

;(() => {
    "use strict";
    const itemsPerRow = 3;
    const query = ".summary-image-wrapper";
    const minHeight = 100; // Maybe a min height just in case

    const row = [];
    var maxHeight = minHeight;

    document.querySelectorAll(query)
        .forEach((imgWrap, i, imgWrappers) => {
            row.push(imgWrap);
            maxHeight = Math.max(imgWrap.firstElementChild.height , maxHeight);

            if (row.length === itemsPerRow || i === imgWrappers.length - 1) {
                row.forEach(imgWrap => imgWrap.style.height = `${maxHeight}px`);
                maxHeight = minHeight;
                row.length = 0;
            }
        });
})();

Answer 2

I like your implementation. It's simple and clear.

I would replace the setting of the row height using a bit more functional style, eliminating the local height variable, and slightly more compact:

row.height = row.elements
  .map(el => el.firstElementChild.getAttribute('height'))
  .reduce((a, b) => a < b ? b : a);

Another thing, I'm not sure if splicing arrays is very efficient. For creating the rows with 3 elements, it may be more efficient to use a counting loop with steps of 3, and working with index ranges instead of splicing. But that would be less idiomatic, and perhaps premature optimization. So I think it's fine as it is.

Answer 3

Feedback

The following line:

row.height = height;

appears to only have merit for the forEach loop just below it. Is there code omitted that utilizes row.height? If not, the forEach could be simplified to use height and the aforementioned line above can be eliminated.

Suggestions

Would you use a sledgehammer to kill a fly?

While it certainly works to use document.querySelectorAll(), that method has a lot more capability than necessary. Because the selector is a simple class name, document.getElementsByClassName() could be used instead, which may allow the code to execute quicker (see jsPerf comparison). Do bear in mind that while the former returns a NodeList, the latter returns a live HTMLCollection so this has a couple ramifications:

• There aren't built-in functional methods (e.g. forEach, map) so to use those, either add the elements in the collection to an array (either using Array.from() or using the spread syntax (e.g. [...elements])), or utilize Function.call on the Array prototype methods passing the collection as the this argument - e.g. Array.prototype.forEach.call(elements, function() { ...).

• Because the collection is live:

  "An HTMLCollection in the HTML DOM is live; it is automatically updated when the underlying document is changed."¹

  The collection can be stored in a variable (perhaps using const) on page load and then re-used whenever the function/functionality is needed.

Because Blindman's answer already lists aspects I would have mentioned like using Math.max(), I would recommend using the code from his example, slightly modified.

Instead of the lines:

document.querySelectorAll(query)
    .forEach((imgWrap, i, imgWrappers) => {

One could also use:

Array.prototype.forEach.call(document.getElementsByClassName('summary-image-wrapper'),
    (imgWrap, i, imgWrappers) => {

Or using the spread syntax to put those elements into an array:

const className = "summary-image-wrapper";
[...document.getElementsByClassName(className)]
    .forEach((imgWrap, i, imgWrappers) => {

See this jsPerf example for a comparison, using 4 image wrapper elements.

---

¹_{https://developer.mozilla.org/en-US/docs/Web/API/HTMLCollection}