HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Showing and hiding text excerpts

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
excerptsshowingtexthidingand

Problem

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 );

Solution

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".

Code Snippets

function () {
  // indent
  // everything
  // inside
  // the function
}
var textW = $headline.width(),
    parentW = $parent.width(),
    ratio = parentW / textW;

// more code
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);
  });
}
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);
  });
}
$(document).on('click', 'img.moreInfo', function(event) {
  // handle click
});

Context

StackExchange Code Review Q#95486, answer score: 4

Revisions (0)

No revisions yet.