Skip to content
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

Add support for rounding to end of sentence #22

Merged
merged 1 commit into from
Sep 2, 2014

Conversation

remy
Copy link
Contributor

@remy remy commented Sep 1, 2014

Use:

downsize("<p>My super long sentence that I don't want chomped.</p><p>But I would like around 75 characters on the page, but I don't want to cut halfway through a sentence.</p>", {
  characters: 75, 
  round: true
}); // result: original two <p> tags

Also includes initial tests (all passing) (could do with more?)

If options.round is used, then as the text is parsed, if it hits the character limit, it also checks to see if it's at the edge of a closing sentence tag . If not, it'll continue to scoop up the text.

If the parser reaches a PARSER_TAG_COMMENCED character (<), then it peeks at the upcoming HTML, and checks if that tag would suggest the end of a sentence, i.e. a </p>, </div>, </li>, etc.

Note: I also tested that it works correctly with unclosed <p> tags, which it does, but the current version of Downsize tries to close all tracked open tags (so if there's more than one automatic closing <p> tag), they're all slapped on the end of the string, resulting in: <p>foo<p>bar</p></p> - this is a completely separate bug which may or may not be worth dealing with.

Includes initial tests (all passing), though could do with more?

If options.round is useds, then as the text is parsed, if it hits the character limit, it also checks to see if it's at the edge of a closing sentence tag . If not, it'll continue to scoop up the text.

If the parser reaches a PARSER_TAG_COMMENCED character (`<`), then it peeks at the upcoming HTML, and checks if that tag would suggest the end of a sentence, i.e. a </p>, </div>, </li>, etc.
@cgiffard
Copy link
Owner

cgiffard commented Sep 2, 2014

Aaah, this is super helpful, and greatly appreciated! Thanks for submitting a PR.

I agree that the tag closing behaviour needs to be fixed, but the question that stands out in my mind is whether a simple whitelist of tags that don't need to be closed would suffice — I think it won't, and there'll need to be some real thought as to how best to balance the document. But I'll get to that in another issue. :)

cgiffard added a commit that referenced this pull request Sep 2, 2014
Add support for rounding to end of sentence
@cgiffard cgiffard merged commit 2eadf3e into cgiffard:master Sep 2, 2014
@adam-zethraeus
Copy link
Contributor

@remy @cgiffard

Hey guys!

From a cursory look, this seems to dupe the functionality of 'contextualTags'.
(i.e. when i wrote contextualTags, it was for the purpose of getting this functionality into ghost.)

Could you take a look at it and confirm/deny this?

i.e. this same effect is available by passing in all of your sentenceTerminatorElements as contextualTags.

it("should await the end of the containing paragraph", function () {
         downsize("<p>there are more than seven words in this paragraph</p><p>this is unrelated</p>", {words: 7, contextualTags: ["p", "ul", "ol", "pre", "blockquote"]})
             .should.equal("<p>there are more than seven words in this paragraph</p>");
     });

if so:

  • I'd be happy to have shortcut functionality for this use of contextualTags (but it might belong in the consumer, not this script. worth debate)
  • but i'd like to revert this commit and not reimplement the same functionality twice.

if i've misunderstood, please let me know!

adam-zethraeus added a commit that referenced this pull request Sep 9, 2014
This reverts commit 2eadf3e, reversing
changes made to e65fe59.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants