patternjavascriptMinor
Showing and hiding text excerpts
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
-
Don't "outdent" when inside a block (i.e. function or control flow construct). Your
-
Don't put a linebreak right after
Personally, I indent following declaration lines by 4 spaces, to make them line up:
Tools like jslint yell at me for doing that, but I find it makes things more readable.
-
Combine your declarations. In
A couple of notes to the above:
Of course, some might say you're overdoing the variable declarations for something rather trivial. This would work just as well:
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:
-
Also, a more jQuery-esque way to write a
-
Be consistent with your whitespace. You have:
and:
but then you have:
I'd stick the latter; the other ones are maybe a bit to "spacey".
-
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 codeTools 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
$headlinefirst, to avoid calling$(this)twice to also get the$parent.
- I've added a zero to
0.99because why not? Yeah, the shorthand works, but it really doesn't do anything. Writing0.99is a perfectly natural way to write a number.
- Oh, and I've renamed the function to use
camelCaseto separate the words, and changeheadingstoHeadlines, 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 codefunction 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.