Skip to main content
Code Review

Return to Revisions

4 of 4
Commonmark migration

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;
 }
 });
})();
Blindman67
  • 22.8k
  • 2
  • 16
  • 40
default

AltStyle によって変換されたページ (->オリジナル) /