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

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.