Fractional balancing; simplification #3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Manipulating the DOM to determine element height is a clever idea, but it's a premature optimization: (1) it's additional complexity, (2) the 10 pixel fudge factor leaves its behavior on small text questionable, (3) this implementation doesn't work when a div's html starts with tags instead of text, because the first split() word ends up nonvisible so firstWord.offsetHeight is zero, and (4) it's faster to simply try squeezing and see what happens. In my rudimentary testing, headlineIsMultipleLines ran in 2.66ms, but simply running squeezeContainer completed in 2.73ms, so it's making things slower. My numbers may not be super accurate, but they would have to be very far off for my conclusion to be backwards.
Function renamed to "squeezeContainerLeft" in anticipation of adding compatibility with centered and right-aligned elements at a later time.
The +1's were gradually shifting the range to the right with each binary subdivision. No real harm but also not correct.
Contributor
|
@jamiemccarthy hey. this looks really cool! Thanks for working on this. We're gonna look into this sucker but I really like your idea. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
These are the changes that Vox Media has used for text-balancer. This version is approximately what's been running on vox.com for some time now.
We'd like to thank you for open-sourcing this library, and submit these changes back upstream for your consideration.
The main change is that there's a new argument to apply only a fraction of the maximum change. On vox.com, we determine how much each headline can be squeezed, and then only squeeze it half that much. We find this to be a visually appealing tradeoff. This new argument is optional and backwards-compatible.
Fractional squeezing
Here is a crude animation to show the effect of different fractional values.
0.00is the same as not enabling text-balancer at all;1.00is the same as text-balancer without this PR applied.Other changes
This PR also proposes a performance improvement and simplification: the removal of
textElementIsMultipleLines. It's a good idea, but in my testing, running this function took about the same time as simply runningsqueezeContainer, so removing it should make things run slightly faster. For details, see the commit message on 6f2b7a7.As an additional performance enhancement, this tweaks the beginning and end of the recursive function to not run quite as long. Squeezing beyond 50% is never necessary because in the only case where it's possible (single line) there is no visual difference. So we start at 50% instead of 0%. And halting the loop once the range is down to 5 pixels saves another couple of iterations at a small cost in accuracy.
Finally, this PR adds a check before trying to squeeze an element, to make sure that its text is left-aligned. The algorithm could be changed to work with center- and right-aligned divs, but as written, its results would look pretty ugly on them. For now, the algorithm should only be applied to those aligned left. It's a simple
getComputedStylecheck, which I think is pretty compatible, though this 2010 comment suggests there's a workaround for an MSIE incompatibility. I don't know how well this works with IE. (Hat tip to @bceskavich for help with this.)