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

TruncateHtml #30

Merged
merged 30 commits into from
Nov 2, 2015
Merged

TruncateHtml #30

merged 30 commits into from
Nov 2, 2015

Conversation

smeeckaert
Copy link

I've implemented a html truncate based FuelPHP truncate function, with breakpoint management. I'm not exactly sure of the code style and hope it's okay.

Changes

This PR add a TruncateHTML class which extends from Truncate, it has the capability to truncate HTML by keeping some tags or removing them all together. It supports the breakpoints and append.

This PR changes the Truncate class by changing the visibility of the properties are refactoring the breakpoint mechanism in order for it to be reusable.

Code example

use Coduo\PHPHumanizer\String;

$text = '<p><b>HyperText Markup Language</b>, commonly referred to as <b>HTML</b>, is the standard <a href="/wiki/Markup_language" title="Markup language">markup language</a> used to create <a href="/wiki/Web_page" title="Web page">web pages</a>.<sup id="cite_ref-1" class="reference"><a href="#cite_note-1"><span>[</span>1<span>]</span></a></sup> <a href="/wiki/Web_browser" title="Web browser">Web browsers</a> can read HTML files and render them into visible or audible web pages. HTML describes the structure of a <a href="/wiki/Website" title="Website">website</a> <a href="/wiki/Semantic" title="Semantic" class="mw-redirect">semantically</a> along with cues for presentation, making it a markup language, rather than a <a href="/wiki/Programming_language" title="Programming language">programming language</a>.</p>';

echo String::truncateHtml($text, 3); // "<b>HyperText</b>"
echo String::truncateHtml($text, 12, ''); // "HyperText Markup"
echo String::truncateHtml($text, 50, '', '...'); // "HyperText Markup Language, commonly referred to as..."
echo String::truncateHtml($text, 75, '<b><i><u><em><strong><a><span>', '...'); // '<b>HyperText Markup Language</b>, commonly referred to as <b>HTML</b>, is the standard <a href="/wiki/Markup_language" title="Markup language">markup...</a>'

@norberttech
Copy link
Member

@smeeckaert nice! Give me some time to check that code.

@norberttech
Copy link
Member

Awesome!
I'm just thinking about how to make it done without chaning visibility of protected fields. I would like to avoid that cuz each private changed into protected is a new extension point that we need to support in future.
So maybe you can create an interface Truncate with one method Truncate::truncate (not quite sure about the name)
That interface would be implemented by TextTruncate (current Truncate) and HtmlTruncate.
About breakpoint method. We can easly extract it as a standalone class and pass to HtmlTruncate and TextTruncate as a dependency.
What do you think about that?

@smeeckaert
Copy link
Author

Yup, that seems a good idea, i will probably make it today, what would be the correct naming of the Interface in the project ? Coduo\PHPHumanizer\String\Truncate\Interface ? or Coduo\PHPHumanizer\String\Interface\Truncate ?

Change string functions into multibyte string functions
@norberttech
Copy link
Member

Well I would like to avoide word "Interface" at all. Maybe just Coduo\PHPHumanizer\String\Truncate or Coduo\PHPHumanizer\String\StringTruncate with following implementations:

  • final class HtmlTrunc implements Truncate
  • final class TextTruncate implements Truncate
  • final class MarkdDownTrunc implements Truncate (I gonna work on that as soon as u finish this PR)

@smeeckaert
Copy link
Author

Okay great.

@smeeckaert
Copy link
Author

Okay, so i've changed the truncate and added an interface.

I've renamed classes to HtmlTruncate and TextTruncate and added a method setBreakpoint to define the breakpoint.

I've created a Breakpoint interface and created a WordBreakpoint class which uses the current mechanism in order to find a breakpoint.

I let only one protected method which is TextTruncate::getLength to reuse the breakpoint mechanism of TextTruncate in HtmlTruncate.

I'm not sure that's 100% what you could want, so tell me if everything is right.

*
* @return int
*/
public function len($text, $charactersCount);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please change name of this method into calculatePosition($text, $charactersCount) ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@norberttech
Copy link
Member

There is also one problem with this PR and we need to think how to solve it. Changing class Truncate into interface Truncate is a BC Break.

What we can do with that:

  • merge this PR into 2.0 version of php-humanier (I can create new branch for that)
  • rename interface Truncate into interface TruncateInterface, then restore class Truncate and make it implement class Truncate implements TruncateInterface also mark it as deprecated and create class WordTruncate extends Truncate

@defrag @smeeckaert what do u guys think?

Norbert Orzechowicz and others added 2 commits October 30, 2015 12:40
…to smeeckaert-master

Conflicts:
	src/Coduo/PHPHumanizer/String/Truncate.php
@smeeckaert
Copy link
Author

We should make Truncate extends WordTruncate, no ? That way the class Truncate keeps its old behaviour.

@norberttech
Copy link
Member

Actually I have been working on that now, please take a look norberttech@93a09b7
There are still some tests failing but this is how I think we can handle adding this feature without BC Breake for current Truncate class.

@smeeckaert
Copy link
Author

Yeah it changes the concept quite extensively (no __toString magic), but yeah, keeping the old code is an idea. But maybe replace the Truncate::construct to instanciate a WordTruncate and using it in the __toString would be a better idea, it's less code to maintain and provides the correct "new way" to do things.

@norberttech
Copy link
Member

The old way with __toString has one problem, Truncate object exists only for one specific text. Its something that I would like to change.
From the usage point of view there is no difference cuz everything is hidden behind the String facade. You can also use HtmlTruncate or WordTruncate as a standalone objects (without facade) and then its gonna be even better cuz you will not need to create new object for each text u need to truncate.

@smeeckaert
Copy link
Author

Yeah, i'm with you, i'm not particularly fan of magic function in general because it hides features, mess with code completion, and force strange-ish syntax, like type-cast in php that is not so common.

I wholehearly agree with this change, more so when we add dependancies like the Breakpoint in the equation.

@norberttech
Copy link
Member

Do you think you could take code that I made and finish it in this branch? I'm quite busy now and dont think gonna have much time until next few weeks

@smeeckaert
Copy link
Author

I'm on it.

@smeeckaert
Copy link
Author

It should be bugfixed, now.

@norberttech
Copy link
Member

wow that was fast! Thanks @smeeckaert, good job!
About the allowed tags, maybe it would be better to not pass any of them by default?

@idmontie
Copy link

Super interested in seeing this added! Currently looking for an alternative to PHPTidy! 👍 🌴

@norberttech
Copy link
Member

@smeeckaert could you please remove default allowed tags? It would be much more flexible to allow users precise their own allowed tags, also its gonna be easier to maintain for us later without any BC Breakes.

@smeeckaert
Copy link
Author

It's done.

@norberttech norberttech merged commit d5df1e4 into coduo:master Nov 2, 2015
@norberttech
Copy link
Member

@smeeckaert thanks for your awesome work! 🍻 I rebased code and merged it manualy.

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.

9 participants