2
\$\begingroup\$

This is a follow-up on Change size of elements until parent reaches certain height

The goal of the function is to find a size for the elements inside a parent, so they all fit in a single row. I tried it with a loop (check previous question), and now I changed it to if statements. However I would like to simplify it, but I can't find how. The Size of the elements don't have to be exactly how they are now. The default size is width: 10px; height: 10px; margin: 5px 7px, which is a total of 24px wide.

function _AdjustHeightOwlDots(id) {
    const thisElementId = `owl-carousel-${id}`;
    const dotRowWidth = $(`#${thisElementId} .owl-dots`).width();
    const amountOfDots = document.querySelector(`#${thisElementId} .owl-dots`).childElementCount;
    const singleDotWidth = $(`#${thisElementId} .owl-dots > button`).width();

    if ( (singleDotWidth * amountOfDots) < dotRowWidth ) { // dots fit in single row
      return;
    }
      let maxDotWidth = Math.floor(dotRowWidth / amountOfDots);
      const setNewStyle = function(hw, margin) {
        $(`#${thisElementId} div.owl-dots > button > span`).css('width', `${hw}px`);
        $(`#${thisElementId} div.owl-dots > button > span`).css('height', `${hw}px`);
        $(`#${thisElementId} div.owl-dots > button > span`).css('margin', `5px ${margin}px`);
      }
      if (maxDotWidth % 2 === 1) {
        maxDotWidth -= 1;
      }

      // A/B
      // width = Apx
      // height = Apx
      // margin = 5px Bpx

      if (maxDotWidth <= 3) {
        // set to 1/1
        setNewStyle(1, 1);
      } else if (maxDotWidth === 4) {
        // set to 2/1
        setNewStyle(2, 1);

      } else if (maxDotWidth === 6) {
        // set to 2/2
        setNewStyle(2, 2);

      } else if (maxDotWidth === 8) {
        // set to 4/2
        setNewStyle(4, 2);

      } else if (maxDotWidth === 10) {
        // set to 4/3
        setNewStyle(4, 3);

      } else if (maxDotWidth === 12) {
        // set to 6/3
        setNewStyle(6, 3);

      } else if (maxDotWidth === 14) {
        // set to 6/4
        setNewStyle(6, 4);

      } else if (maxDotWidth === 16) {
        // set to 8/4
        setNewStyle(8, 4);

      } else if (maxDotWidth === 18) {
        // set to 8/5
        setNewStyle(8, 5);

      } else if (maxDotWidth === 20) {
        // set to 10/5
        setNewStyle(10, 5);

      } else if (maxDotWidth === 22) {
        // set to 10/6
        setNewStyle(10, 6);
      }
  };
\$\endgroup\$
3
  • \$\begingroup\$ What is {{!block.id}}? \$\endgroup\$ – RoToRa Dec 11 '19 at 9:34
  • \$\begingroup\$ @RoToRa It's a python variable from the templating engine. I removed it from the code it was confusing \$\endgroup\$ – Gautama Dec 11 '19 at 9:58
  • \$\begingroup\$ Please don't change code after posting it. My review referrers to it. \$\endgroup\$ – RoToRa Dec 11 '19 at 11:19
3
\$\begingroup\$

Using a template variable in the script means you have this script in the HTML document and repeat it for each owl carousel. That is a vary bad idea, mostly because that way you may have multiple functions with the same name.

Generally your HTML shouldn't "know about" (reference) your JavaScript. Instead the JavaScript should find the elements it wants to attach itself to.

Put the function in a separate script file (together with all of your other scripts, wrapped in an IIFE) and include it once with <script src="..."> at the end of the HTML.

Then place a class on all owl carousels you want to apply the function to and use that to call the script for all and any elements with this class. For example, with jQuery:

(function() {

    $('.owl-carousel--adjust-height').each(function () {
        _AdjustHeightOwlDots($(this));
    };

    function _AdjustHeightOwlDots(owlCarousel) {
        // ...
     }

})();

Now, inside "find" the sub elements (and save references to avoid duplicate queries of the same elements):

const dots = owlCarousel.find('.owl-dots');
const dotRowWidth = dots.width();
const amountOfDots = dots[0].childElementCount;
const dotButtons = dots.find('> button');
const singleDotWidth = dotButtons.width();

(You learn new things every day. Never heard of childElementCount before this - and it's not even a new thing.)

In setNewStyle the query is also repeated and can be replaced with chaining (or the alternative syntax for .css() using an object) and also can be moved outside the function:

const dotButtonSpans = dotButtons.find('> span');

const setNewStyle = function(hw, margin) {
    dotButtonSpans
        .css('width', `${hw}px`)
        .css('height', `${hw}px`)
        .css('margin', `5px ${margin}px`);
}

Or

const setNewStyle = function(hw, margin) {
    dotButtonSpans.css({
        width: `${hw}px`, 
        height: `${hw}px`, 
        margin: `5px ${margin}px`
    });
}

Finally the big if block can be replaced with a simple calculation (including removing the part that makes maxDotWidth even, instead using | 0 as a short cut to round numbers to integers):

const size = ((maxDotWidth / 4) | 0) + 1;
const margin = maxDotWidth < 2 ? 1 : (((maxDotWidth - 2) / 4) | 0) + 1;

setNewStyle(size , margin);
\$\endgroup\$
1
  • \$\begingroup\$ Hey thanks for the response, I implemented your feedback, however I changed the calculations of the size to const size = maxDotWidth <= 2 ? 1 : (maxDotWidth - margin * 2); this way I don't waste any pixels! \$\endgroup\$ – Gautama Dec 11 '19 at 15:24
2
\$\begingroup\$

Some remarks on chains of if (...) else if (...) calling same function. You can replace them with swich case which also is not ideal. What I like to do is something like this:

let styleDataList = {
    4:  { hw: 2,  margin: 1 },
    6:  { hw: 4,  margin: 2 },
    8:  { hw: 4,  margin: 2 },
    10: { hw: 4,  margin: 3 },
    12: { hw: 6,  margin: 3 },
    14: { hw: 6,  margin: 4 },
    16: { hw: 8,  margin: 4 },
    18: { hw: 8,  margin: 5 },
    20: { hw: 10, margin: 5 },
    22: { hw: 10, margin: 6 }
};

let styleData = styleDataList[maxDotWidth] || { hw: 1, margin: 1 };
setNewStyle( styleData );

But yes, as @RoToRa pointed out - this whole structure can most likely be reduced to a simple calculation.

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