0
\$\begingroup\$

On scroll, I am adding a class (current) to particular elements.

The code looks like it can be made better. For example, it seems like I am reusing the same things over and over again (i.e. the nth-child(1), nth-child(2), etc.).

I appreciate any and all comments and advice.

 function isScrolledIntoView(elem) {
    if (elem.length > 0) {
      var docViewTop = $(window).scrollTop();
      var docViewBottom = docViewTop + $(window).height();
      var elemTop = $(elem).offset().top;
      return ((elemTop <= docViewBottom) && (elemTop >= docViewTop));
    }
  }

   $(document).scroll(function () { // Measure scroll To Top
      // @TODO Fix it so that there is only 1 if block
      if (isScrolledIntoView($('.section .main-content:nth-child(1)')) === true) {
        $('.events-widget-menu li').removeClass('current');
        $('.events-widget-menu li:nth-child(1)').addClass('current');
      }

      if (isScrolledIntoView($('.section .main-content:nth-child(2)')) === true) {
        $('.events-widget-menu li').removeClass('current');
        $('.events-widget-menu li:nth-child(2)').addClass('current');
      }

      if (isScrolledIntoView($('.section .main-content:nth-child(3)')) === true) {
        $('.events-widget-menu li').removeClass('current');
        $('.events-widget-menu li:nth-child(3)').addClass('current');
      }

      if (isScrolledIntoView($('.section .main-content:nth-child(4)')) === true) {
        $('.events-widget-menu li').removeClass('current');
        $('.events-widget-menu li:nth-child(4)').addClass('current');
      }
    });
\$\endgroup\$
1
  • \$\begingroup\$ Try $('.section .main-content').filter(function() { return isScrolledIntoView($(this)); }).last().addClass('current'); in ready(). \$\endgroup\$
    – Tushar
    Jan 25, 2017 at 8:31

2 Answers 2

1
\$\begingroup\$

I don't understand why :nth-child selector is even in play here. It seems all you are wanting to do is add a class to an li with same index within its parent as what the content's index within it's parent is.

I might suggest something along these lines to achieve that:

// Cache jQuery collections somewhere in scope that `scroll()` inherits.
// This prevents you from having to continually re-query the DOM every time
// there is a scroll event.
var $mainContent = $('.section .main-content');
var $eventLi = $('.events-widget-menu li');

$(document).scroll(function () {
    $eventLi.removeClass('current');
    // find content elements in view
    var $currentElements = $mainContent.filter(function(index) {
        return isScrolledIntoView(this);
    });
    // apply class to LI with corresponding index
    $currentElements.each(function() {
        var index = $mainContent.index(this);
        $eventLi.eq(index).addClass('current');
    });
}

This solution is not hard-coded to X number of elements.

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

You probably want something like:

$(document).scroll(function () {
  for (i = 1; i <= 4; i++) {
      var curElem = $('.section .main-content:nth-child(' + i.toString() + ')');
      if (isScrolledIntoView(curElem) === true) {
        $('.events-widget-menu li').removeClass('current');
        curElem.addClass('current');
      }
    }
});

I didn't test it, but that should be it.

\$\endgroup\$
1
  • \$\begingroup\$ Without a declaration for i using var or let a global variable would be created, which won't lead to an error but typically frowned upon \$\endgroup\$ Sep 20, 2020 at 5:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.