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

Move fix for #1114 to Truncator::truncateLetters #2004

Merged
merged 2 commits into from
May 15, 2018

Conversation

dliessi
Copy link
Contributor

@dliessi dliessi commented May 2, 2018

The original fix provided by #1125 in Utils::truncateHTML compared the
summary size with the full HTML string length.
Thus, the string was still being truncated (and the HTML rewritten) even
when only the HTML string length, and not the text length, exceeded the
summary size.

@dliessi
Copy link
Contributor Author

dliessi commented May 2, 2018

The failing test is

$this->assertEquals('<input type="file" id="file" multiple>', Utils::truncateHtml('<input type="file" id="file" multiple />', 6));

I suspect that the first argument of assertEquals was chosen to be different from the first argument of Utils::truncateHtml only because Truncator::innerHtml rewrites the HTML.
Is this true? Can I make them equal? Which form should be retained (with or without the closing /)?

The original fix provided by getgrav#1125 in Utils::truncateHtml compared the
summary size with the full HTML string length.
Thus, the string was still being truncated (and the HTML rewritten) even
when only the HTML string length, and not the text length, exceeded the
summary size.
@dliessi
Copy link
Contributor Author

dliessi commented May 4, 2018

I chose to keep the XHTML syntax, based on other code generated by Grav, although in HTML it is unnecessary.

@rhukster rhukster merged commit a1abcfd into getgrav:develop May 15, 2018
@rhukster
Copy link
Member

looks good.. cheers.

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.

2 participants