-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Big rework, don't split on lowercase words #2
Conversation
Pretty big rework of the plugin. Details in the PR.
I'd love to use this plugin as well, but then for another language (Dutch). Not splitting after lowercase words is a bit tricky because in Dutch the words are mostly not capitalized. It would be great to make this feature an optional one. Btw, maybe it is easier if there's separate PRs for each change, or otherwise at least separate commits. I found it hard to read what changed with this PR and how the lowercase-check affected generic usage. And I guess reviewing is also tricky this way. |
@lode Great idea, I didn't even think about that even though I'm Dutch myself. I'll add it as an option. (quick side note: what site are you planning to use it for? Just curious!). As for it being hard to read and review, good point. I'll give an overview of what changed and how it changed, that should hopefully help both with understanding and reviewing. As for how it changed generic usage, the only thing that changed is that if after balancing the text the plugin finds that the last word on the first line is lowercase and the word after that isn't, it increases the width of the wrapper to fit both words on the same line. Overview of changes:
Individual functions:
Sorry for the wall of text, but I hope this helps! Let me know if you have any questions. |
@lode It's an option now, so if you wanted to avoid splitting on lowercase words you'd do this: textBalancer.init({
candidates: ['.headline'],
splitOnLowercase: false
}); In your case you don't need the option since it's disabled by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielpost things are looking good!
The two biggest things I want to see from this refactor are that text-balancer is backwards compatible, and that it's easily testable.
As far as the backwards compatibility, what do you think about reworking this so someone doesn't have to call init()
immediately, but could call balanceText()
?
What do you think? I could totally be missing something, these are just some initial thoughts :^)
element.style.maxWidth = Math.ceil(topRange + 1) + 'px'; | ||
|
||
if (!settings.splitOnLowercase) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be defensive here and check to see if settings exists first! Don't want this to break if someone doesn't pass in settings :^)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That setting is passed in by default as true so it should be fine! I'll take a look at the other stuff soon.
I really like this PR! And today I learned about Unfortunately this doesn't help news sites (like the one I work for) that use ordinary sentence case instead of capitalizing most words. But I'd still like to see this approved and merged, it takes the library in a very interesting new direction. As a next step, I might like to experiment with several things:
to:
we could see whether these fit:
(Perhaps simultaneously expanding the width by a small fixed amount would make this type of change more likely to succeed with little negative visual effect.)
is not as readable as this:
even if it has to go a little wider.
|
@jamiemccarthy Is this something that would still interest you? I kind of forgot about it and this library seems mostly abandoned so I'm considering developing a modern fork. |
Hey guys,
I wanted to add the ability to not break on lowercase words (as mentioned in your article), and ended up with a pretty big structural overhaul of the entire plugin. Here's what I added:
If the last word before the split starts with a lowercase letter and the word after that doesn't, the width will be increased to fit the next word as well. This was my initial idea and kind of prompted the rest.
Right now only candidates is useful, but it might be nice to eventually add more options, and this creates an easy way to add new options.
I changed the structure to be more consistent with a regular vanilla JavaScript plugin, and added or improved some comments to hopefully improve clarity.
Initialize like this:
I realize this is a pretty major (and opinionated) overhaul and I would love to hear your thoughts/suggestions/ideas!
Best,
Daniel