2
\$\begingroup\$

Making long story short, I got a chance to jump from DevOps to Software Development (Frontend). Because I'm quite new to JS design patterns, I need some quality advice about the job I've done recently.

The goal is to create a component which checks if there's a particular class on the current page, and if yes, add event listeners for the document (one default & one custom) that will hold the logic for triggering a popup. The popup should appear only once, then the event listener should not perform any anymore actions. For this, I defined the lockPopup variable (state management). There is also screenEdge flag available, since the requirements for this project are changing and it basically flips around quite often.

I chose a kind-of revealing module pattern, with init function that will be called on jQuery's $( document ).ready() method.

var Popup = (function() {

  // Component state management
  var lockPopup = false;
  var screenEdge = 'top';

  // Variables for jQuery selectors
  var passableClassName = '.passable-class';
  var animatedClassName = '.animated-class';

  // Cache DOM
  var $elementToSelect = $(passableClassName);
  var $elementToAnimate = $(animatedClassName);

  // Entrypoint for application
  function _init() {
    _conditionalEventBind();
  }

  // Conditional event binding
  function _conditionalEventBind() {
    if (typeof $elementToSelect[0] !== 'undefined') {
      $(document).scroll(_notifyEventToBeDispatched);
      $(document).on("togglePopup", _togglePopup);
    }
  }

  // Event listener callbacks
  //
  // Determining the position from selected item in document
  function _determineValidPosition() {
    var $scrolledDistance = $(window).scrollTop();
    var $itemTopOffset = $elementToSelect.offset().top;
    if (screenEdge === 'top') {
      return $scrolledDistance >= $itemTopOffset;
    };
    if (screenEdge === 'bottom') {
      return ($scrolledDistance + window.screen.availHeight) >= $itemTopOffset;
    }
  }

  function _notifyEventToBeDispatched() {
    if (!lockPopup) {
      if (_determineValidPosition()) {
        $.event.trigger({   type: "togglePopup" });
        lockPopup = true;
      }
    }
  }

  // Function definition for popup animation
  function _togglePopup() {
    $($elementToAnimate).fadeIn(500).delay(5000).fadeOut(500);
  }

  // **************************** Component initialisation *
  // Returning main entry point for application, so the rest of
  // component features are private & not accessible from outside
  return {
    init: _init
  }
})();

Example how will be called:

$( document ).ready(function() {
    Popup.init();
});

I will be extremely grateful for any hints regarding this style/logic that I've applied here.

\$\endgroup\$
7
  • \$\begingroup\$ a quicker way to do typeof $elementToSelect[0] !== 'undefined' is just if($elementToSelect.length) .. if it doesn't have any elements the length will be zero, which is falsy. \$\endgroup\$ Commented Aug 21, 2017 at 20:25
  • \$\begingroup\$ your init function is pointless. alls it does is call another function. just rename the other function init if you so desperately need to have a function with that name. \$\endgroup\$ Commented Aug 21, 2017 at 20:27
  • \$\begingroup\$ nested if statements in _notifyEventToBeDispatched .. combine them, if (!lockPopup && _determineValidPosition())... or better, inverse them.. if(lockPopup || !_determineWhaterver()) return; and then the rest of the code can follow without being nested. \$\endgroup\$ Commented Aug 21, 2017 at 20:32
  • \$\begingroup\$ Cheers for good hints, indeed this code has some unnecessary bits & it can be simplified. I have got another question. What if I encounter early code execution & selectors are not going to be picked up, so will be always undefined? How could I possibly make it being executed after DOM is loaded? I have seen some examples with nesting $(document).ready inside of it, but then is it not bit confusing? \$\endgroup\$
    – xsimonzx
    Commented Aug 22, 2017 at 6:42
  • \$\begingroup\$ Yea, there's no reason why you can't nest a document.ready in there except that really, you should just be making sure you call the popup.init function in a document.ready call. \$\endgroup\$ Commented Aug 22, 2017 at 11:31

0

Your Answer

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