1
\$\begingroup\$

I'm just a bit confused and can not write or simplify a jQuery code, that adds or removes classes based on the viewport size. I'm pretty sure there's a simpler and more efficient way to write that trivial code. Unfortunately I can not get it.

Here is the current status:

    // initial state
    if ($(window).width() <= 900) {
        $('.readmoresection span.handler').show();
        $('.readmoresection').addClass('reduced');
    }else{
        $('.readmoresection span.handler').hide();
        $('.readmoresection').removeClass('reduced');
    }       

    // on resize
    $(window).on('resize', function(){
        if ($(window).width() <= 900) {
            $('.readmoresection span.handler').show();
            $('.readmoresection').addClass('reduced');
            if($('.readmoresection').hasClass('reduced')){
                $('.readmoresection span.handler').html(' » Weiterlesen');
            }else{
                $('.readmoresection span.handler').html(' » Schließen');
                $('.readmoresection').removeClass('reduced');
            }
        }
        else{
            $('.readmoresection span.handler').hide();
            $('.readmoresection').removeClass('reduced');
        }   
    });

Does anyone know a cleaner way?

\$\endgroup\$

migrated from stackoverflow.com Dec 2 '17 at 0:12

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ if($('.readmoresection').hasClass('reduced')) is always true since you add the calss right before you check the condition. this is probably not the desired behavior \$\endgroup\$ – vbranden Dec 1 '17 at 23:50
  • \$\begingroup\$ For each resize, I have to check if the condition is true and adjust the innerHTML --> html(' » Weiterlesen'); accordingly. It has only worked in this way so far. \$\endgroup\$ – Codehan25 Dec 2 '17 at 0:02
1
\$\begingroup\$

Using extract method to avoid redundancy

function onResizeReadMoreLayout(){
if ($(window).width() <= 900) {
            $('.readmoresection span.handler').show();
            $('.readmoresection').addClass('reduced');
            if($('.readmoresection').hasClass('reduced')){
                $('.readmoresection span.handler').html(' » Weiterlesen');
            }else{
                $('.readmoresection span.handler').html(' » Schließen');
                $('.readmoresection').removeClass('reduced');
            }
        }
        else{
            $('.readmoresection span.handler').hide();
            $('.readmoresection').removeClass('reduced');
        }  
}



// initial state
 onResizeReadMoreLayout();
 // on resize
 $(window).on('resize', onResizeReadMoreLayout);
\$\endgroup\$
  • \$\begingroup\$ Your solution is somehow the only one that works in my case. Find it also very clear and clean. Thanks a lot for this :) \$\endgroup\$ – Codehan25 Dec 6 '17 at 8:37
1
\$\begingroup\$

I've used matchMedia() quite a bit in the past, it works quite well: https://github.com/paulirish/matchMedia.js/

Example:

if (matchMedia('only screen and (max-width: 480px)').matches) {
  // smartphone/iphone... maybe run some small-screen related dom scripting?
}

Also instead of individually doing .addClass('class') and .removeClass('class') you could use .toggleClass('class').

\$\endgroup\$
  • \$\begingroup\$ this definitely doesnt answer the question. \$\endgroup\$ – vbranden Dec 1 '17 at 23:52
1
\$\begingroup\$

You could reduce complexity via reducing the overall count of function calls to $. There is only one bit of missing information. The state of style.display of the handler span which will be in the CSS. As it is not clear when the code runs I can not assume it is in its initial state.

;(()=>{
    const el = {
        handler : document.querySelector(".readmoresection span.handler"),
        element : document.querySelector(".readmoresection"),
    };
    function resize(){
        if(innerWidth < 900){
            el.handler.style.display = "none";
            el.element.classList.add("reduced");
            el.handler.textContent = " » Weiterlesen";
        }else{
            el.handler.style.display = ???;   // << unknown information
            el.element.classList.remove("reduced");
            el.handler.textContent = " » Schließen";
        }
    }
    resize(); 
    addEventLisener("resize",resize);
})();
\$\endgroup\$
  • \$\begingroup\$ Unfortunately it does not work. In this way, the logic of the classes is no longer correct. They are not properly added and also the terms within .html () are not correct. Your solution does not respond to the viewport size. \$\endgroup\$ – Codehan25 Dec 6 '17 at 8:32
  • \$\begingroup\$ @Codehan25 Yes that is because I had no reference to the CSS, did not know initial states. I set textContent rather than HTML as I assumed you were not overwriting actual DOM elements, I used innerWidthbut I don't know your layout. Oh well you will have to continue using jQuery, however I would say it is well worth doing it the vanilla way as it greatly simplifies the code and give significant performance benefits. The above code has only 6 function calls, while your OP has ~31 function calls (excluding hidden jQuery internal calls) \$\endgroup\$ – Blindman67 Dec 6 '17 at 12:42
1
\$\begingroup\$

I tend to use toggle instead of show/hide and toggleClass instead of add/removeClass. Something like:

// initial state
var small = $(window).width() <= 900;
$('.readmoresection span.handler').toggle(small);
$('.readmoresection').toggleClass('reduced', small);

// on resize
$(window).on('resize', function() {
    var small = $(window).width() <= 900;
    $('.readmoresection span.handler').toggle(small);
    $('.readmoresection').toggleClass('reduced', small);
    if (small) {
      // note that, as someone else pointed else reduced will always be
      // true here and the `removeClass('reduced') is also redundant
      var reduced = $('.readmoresection').hasClass('reduced');
      $('.readmoresection span.handler').html( reduced ? ' » Weiterlesen' : ' » Schließen');
    }
});

You could also replace your initial state code with $(window).resize(); or $(window).trigger('resize');

I do think, however, that you would be better off doing this in CSS using a media selector.

\$\endgroup\$
0
\$\begingroup\$

The first thing you can do is cache the two jQuery selectors you're using. It'll save time on the multiple lookups. Since a lot of your functionality is identical on page load and resize you can consolidate it in one function and call it as needed (keeps things nice and DRY).

I also added a few default variables: one, a variable to set whether or not to fire your resizing logic and another for your breakpoint value in the event you want to change it in the future.

$handler = $('.readmoresection span.handler');
$readMoreSection = $('.readmoresection');

toggleState();

$(window).on('resize', toggleState.bind(null, true));

function toggleState(resizing = false, breakpoint = 900) {
 if (window.width <= breakpoint) {
   $handler.show();
   $readMoreSection.addClass('reduced');

   if (resizing) {
     if ($readMoreSection.hasClass('reduced')) {
       $handler.html(' » Weiterlesen');
     } else {
       $handler.html(' » Schließen');
       $readMoreSection.removeClass('reduced');
     }
   }
 } else {
   $handler.hide();
   $readMoreSection.removeClass('reduced');
 }
}
\$\endgroup\$
  • \$\begingroup\$ Have tried your suggestion. However, nothing works now. The readmoresection is no longer visible and the classes are not added properly. \$\endgroup\$ – Codehan25 Dec 6 '17 at 8:27

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.