Showing and hiding text excerpts

Question

Below is my jQuery scriptfile, all the functionality works (except for the click events on IE). I bring this before you guys to ensure I am effectively separating concerns and writing efficient scripts.

( function( $ ) {

  $( document ).ready(function() {
    setTimeout(textFill, 50);
    fitheadliner();
    showExcerpt();
    hideExcerpt();
  });

  $(window).resize( function() {
    fitheadliner();
    textFill();
  });

  function showExcerpt() {
    $('img.moreInfo').each(function() {
      var $excerpt = $(this),
        $excerptParent = $excerpt.parent();

      $excerpt.click(function() {
        var $boxInfo = $excerptParent.children('.modalExcerpt');
        $boxInfo.css("display", "initial");
      });
    });
  }

  function hideExcerpt() {
    $('.xBox').each(function() {
      var $thisBox = $(this);
      $thisBox.click(function() {
        $thisBox.parent().css("display", "none");
      });
    });

    $('.modalExcerpt').mouseleave(function() {
      if ($(window).width() > 700) {
        $(this).css("display", "none");
      };
    });
  }

  function fitheadliner() {
    $(".bigHeadline").each(function() {
    var
      $parent = $(this).parent(),
      $headline = $(this);

    var
      textW = $headline.width(),
      parentW = $parent.width(),
      ratio = parentW / textW;

    var
      originalSize = parseFloat($headline.css('font-size')),
      newSize = originalSize * (.99 * ratio);

      $headline.css("font-size", newSize);
    });
  }

function textFill() {
  $(".textFill").each(function() {
    var $text = $(this),
      $parent = $text.parent(),
      textW = $text.width(),
      textH = $text.height(),
      parentW = $parent.width(),
      parentH = $parent.height(),
      ratio = (parentW + parentH) / (textW + textH),
      originalSize = parseFloat($text.css('font-size'));

     var
      newSize = originalSize * (.9 * ratio);

    $text.css("font-size", newSize);

  });
}

} )( jQuery );

Answer 1

Looks good overall. Only a few minor quibbles:

• Comments. Always a good idea. Write some.

• What's with the show-and-immediately-hide-excert stuff in the ready event handler? I have no idea (I no comments to help me).

• Don't "outdent" when inside a block (i.e. function or control flow construct). Your var declarations should be indented like anything else.

  function () {
    // indent
    // everything
    // inside
    // the function
  }
  

• Don't put a linebreak right after var; put the first variable on the same line.
  Personally, I indent following declaration lines by 4 spaces, to make them line up:

  var textW = $headline.width(),
      parentW = $parent.width(),
      ratio = parentW / textW;
  
  // more code
  

  Tools like jslint yell at me for doing that, but I find it makes things more readable.

• Combine your declarations. In fitheadliner you have 3 groups of var declarations in a row. I understand the grouping - it makes sense - but I'd either use a separate var for each line, or one big chunk. If you want to keep the grouping, you can separate declaration and assignment, e.g.:

  function fitHeadings() {
    $(".bigHeadline").each(function() {
      var $headline, $parent,
          textWidth, parentWidth, ratio,
          originalSize, newSize;
  
  
      $headline = $(this);
      $parent = $headline.parent();
  
      textWidth = $headline.width();
      parentWidth = $parent.width();
      ratio = parentWidth / textWidth;
  
      originalSize = parseFloat($headline.css('font-size'));
      newSize = originalSize * (0.99 * ratio);
  
      $headline.css("font-size", newSize);
    });
  }
  

  A couple of notes to the above:

  • I've assigned $headline first, to avoid calling $(this) twice to also get the $parent.
  • I've added a zero to 0.99 because why not? Yeah, the shorthand works, but it really doesn't do anything. Writing 0.99 is a perfectly natural way to write a number.
  • Oh, and I've renamed the function to use camelCase to separate the words, and change headings to Headlines, since a) it's plural, and b) they're headings, not "headliners".

  Of course, some might say you're overdoing the variable declarations for something rather trivial. This would work just as well:

  function fitHeadlines() {
    $(".bigHeadline").each(function() {
      var $this = $(this), $parent,
          ratio = $this.parent().width() / $this.width(),
          newSize;
  
      newSize = 0.99 * ratio * parseFloat($headline.css('font-size'));
      $headline.css("font-size", newSize);
    });
  }
  

  By the way, why multiply by 0.99? Again, comments would be nice.

• A more jQuery-esque way to attach the same event handler to multiple elements would (in most situations) be to attach the handler to a common parent object, and use the selector as a filter:

  $(document).on('click', 'img.moreInfo', function(event) {
    // handle click
  });
  

• Also, a more jQuery-esque way to write a document ready handler is to just say:

  $(function () {
    // this is run on document ready
  )};
  

• Be consistent with your whitespace. You have:

  ( function( $ ) {
  

  and:

  $( document ).ready(...)
  

  but then you have:

  $(window).resize(...)
  

  I'd stick the latter; the other ones are maybe a bit to "spacey".

Answer 2

I think like Flambino mentions, we would be well served by some comments. But here are a few of my observations:

1. You are using a self-executing anonymous function. This is awesome for encapsulation and fighting memory leaks, so well done IF this is your intended purpose. If the entire contents of the script are meant to be "do this once, bind some events, and then forget about it" then great. But that brings me to #2:

2. I don't see many opportunities for re-using code here. As soon as you find yourself wanting to fit a headliner, hide excerpts, or show excerpts again, you will want to break out of this all-encompassing self-executing function and create an "application" that has methods which will live on in a namespace.

3. It's not clear why there's a setTimeout on the textFill. I cannot tell without comments when in the "flow of events" the first textFill is supposed to happen, but using a timeout to beat a race condition is not good practice. You should identify which completion action should fire the textFill and call it appropriately.

4. I have found $elem.parent() to be future-resistant in my code. As soon as you change your markup structure (for whatever reason), the parent could change and depending on when that revision happens, it might be a bit of a hunting expedition to find out why the code broke! However, it is somewhat more likely that you'll have a container of a particular class or element type. Your mileage may vary, but for my own code, I prefer using $elem.closest(). Note, you may also want to revise semantics to avoid calling it the 'parent' but that's also up to you (note, you were also missing a var declaration for $excerptParent which was putting it into the global scope):

Syntax:

var $container = $excerpt.closest('.containerName');

Along the same lines, you may want to use $elem.find() instead of $elem.children(), so that it can traverse the DOM deeper if your structure changes:

var $boxInfo = $container.find('.modalExcerpt');

Because there's more potential DOM to traverse, I'm guessing it's not quite as performant, but we're talking hundreds of thousands of operations per second either way.

5. Instead of using $elem.css, consider using classes and $elem.addClass(). If you truly don't think it's worth potentially polluting your stylesheet, you get to make that decision. However, a nice semantic class that does what you want it to do makes for readable code AND better maintenance. Your JavaScript gets to say "Make this a disabledExcerpt" which the stylesheet may simply set the element to have display:none; for now, but in the future maybe it's greyed-out and italicized or something.

6. I do not personally favour the $elem.click() method for binding the event handler. Too often I have written methods that modify the DOM one way or another, and the handler is lost. Instead, bind the listener to the nearest ancestor that you can predict with pretty good certainty won't get destroyed. If you are absolutely positive that you are never going to lose your elements that are listening for their own clicks, then you're fine already. Otherwise, here's the pattern:

HTML:

<div class="container">
  <div class="someContent">
    <p>Foo to the bar to the baz.</p>
    <button class="clickerator">Click me!</button>
  </div>
</div>

JS:

$('.container').on('click', '.clickerator', function(e) {
  e.preventDefault(); // well, IF you want to prevent default behaviour...
  alert('Hi, I am doing stuff on click of clickerators!');
})

The JS event binding can be read in plain English thus: "On click of clickerators found in containers, do this function". The container is the one listening for the clicks.

Once again, it's not quite AS performant, and the higher up in the tree your ancestor is, it becomes even less so. That said, people do just fine with highly dynamic websites when they throw their hands in the air and say, "heck with this, my listener will be the document itself... for ALL THE EVENTS!" There comes a time when performance is an academic exercise and the soft costs of development are worth more. I personally do not bind listeners to document, but there's usually at LEAST one container ancestor on a given page that you can be assured will not be destroyed.