EZP-29554: As an editor, I want to be able to apply custom styles to part of my text #2427

erdnaxelaweb · 2018-08-21T14:16:12Z

Question	Answer
JIRA issue	https://jira.ez.no/browse/EZP-29554
Bug/Improvement	yes
New feature	yes
Target version	`7.x`
BC breaks	no
Tests pass	no
Doc needed	yes

TODO:

Implement feature / fix a bug.
Implement tests.
Fix new code according to Coding Standards ($ composer fix-cs).
Ask for Code Review.

Added a way to declare and use custom "styles" in richtext (https://alloyeditor.com/docs/features/styles.html)

PR for the admin ui : ezsystems/ezplatform-admin-ui#602

Configure the custom style

ezpublish:
    system:
        default:
            fieldtypes:
                ezrichtext:
                    custom_styles: [highlighted_block, highlighted_word]
    
    ezrichtext:
        custom_styles:
            highlighted_word:
                template: '@ezdesign/field_type/ezrichtext/custom_style/highlighted_word.html.twig'
                inline: true
            highlighted_block:
                template: '@ezdesign/field_type/ezrichtext/custom_style/highlighted_block.html.twig'
                inline: false

Default templates

Block template :

<div class="{% if align is defined %}align-{{ align }}{% endif %} style-{{ name }}">{% spaceless %}{{ content|raw }}{% endspaceless %}</div>

Inline template :

<span class="style-{{ name }}">{% spaceless %}{{ content|raw }}{% endspaceless %}</span>

Labels

Custom styles label are generated using SF translation.
Translation domain is "custom_styles".

ezrichtext.custom_styles.highlighted_block.label: Highlighted block
ezrichtext.custom_styles.highlighted_word.label: Highlighted word

crevillo · 2018-08-21T14:53:32Z

eZ/Publish/Core/MVC/Symfony/FieldType/RichText/Renderer.php

+        }
+
+        if (!$this->templateEngine->exists($templateName)) {
+            if (isset($this->logger)) {


nitpick, how about a dedicate method for this logging part in order to avoid some dupe code?

Agree.
I think i would be best also to move each "render" to dedicated class as this one keep growing.

nitpick, how about a dedicate method for this logging part in order to avoid some dupe code?

Actually, that's not what we do nowadays.
Please change in the constructor:

$this->logger = $logger;

to

$this->logger = $logger ?? new NullLogger();

and then you can skip if (isset($this->logger)).

I think i would be best also to move each "render" to dedicated class as this one keep growing.

Yes. If you're up to it you can do it, but as this is not necessarily in the scope of this PR, for now it can stay like this.

alongosz

Looks okay-ish. A lot of work, good job @florianalexandre 👍

First and foremost I miss test cases:

You need to create proper fixtures in the eZ/Publish/Core/FieldType/Tests/RichText/Converter/Xslt/_fixtures directory so we can see in a clear way what XSL transforms here.
There is also missing Style Converter test in the eZ/Publish/Core/FieldType/Tests/RichText/Converter/Render directory.

What worries me a bit is a lot of failing tests on Travis. It needs to be addressed also before we proceed. In case of REST/Behat jobs just a rebase might be needed.

Other requests:

alongosz · 2018-08-31T09:00:31Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/FieldType/RichText/style/default.html.twig

@@ -0,0 +1 @@
+<div class="{% if align is defined %}align-{{ align }}{% endif %} style-{{ name }}">{% spaceless %}{{ content|raw }}{% endspaceless %}</div>


Missing new line at the end of the file

alongosz · 2018-08-31T09:01:19Z

eZ/Bundle/EzPublishCoreBundle/Resources/views/FieldType/RichText/style/default_inline.html.twig

@@ -0,0 +1 @@
+<span class="style-{{ name }}">{% spaceless %}{{ content|raw }}{% endspaceless %}</span>


Same here - missing newline (you can only see it on github preview probably).

alongosz · 2018-08-31T09:22:41Z

eZ/Publish/Core/MVC/Symfony/FieldType/RichText/Renderer.php

+        }
+
+        if (!$this->templateEngine->exists($templateName)) {
+            if (isset($this->logger)) {


nitpick, how about a dedicate method for this logging part in order to avoid some dupe code?

Actually, that's not what we do nowadays.
Please change in the constructor:

$this->logger = $logger;

to

$this->logger = $logger ?? new NullLogger();

and then you can skip if (isset($this->logger)).

I think i would be best also to move each "render" to dedicated class as this one keep growing.

Yes. If you're up to it you can do it, but as this is not necessarily in the scope of this PR, for now it can stay like this.

alongosz · 2018-08-31T10:00:24Z

eZ/Publish/Core/FieldType/RichText/Resources/stylesheets/docbook/xhtml5/edit/core.xsl

@@ -627,6 +627,34 @@
    </xsl:element>
  </xsl:template>

+  <!-- Custom template tag code -->


copy-paste typo I guess, should be Custom style tag code, right? (the "template" doesn't refer to XSL markup but to eZ templates which are more widely known as Custom Tags)

alongosz · 2018-08-31T10:13:06Z

eZ/Publish/Core/FieldType/RichText/Resources/stylesheets/xhtml5/edit/docbook.xsl

@@ -622,4 +622,27 @@
    <xsl:value-of select="translate( substring-before( substring-after( concat( substring-after( $style, $property ), ';' ), ':' ), ';' ), ' ', '' )"/>
  </xsl:template>

+  <xsl:template match="ezxhtml5:div[@data-style]">


Hmm, this is about our custom styles, not <div style attribute, right? If so, then I'd prefer to see here ezstyle.

erdnaxelaweb · 2018-09-09T18:52:59Z

Fixed requested change from @alongosz and failed tests

ViniTou

Lot of parameters and return values could be typehinted here.

eZ/Bundle/EzPublishCoreBundle/Resources/config/default_settings.yml

mikadamczyk · 2018-09-11T11:08:03Z

eZ/Publish/Core/FieldType/RichText/Converter/Render/Style.php

+     */
+    protected function getNodeDepth(DomNode $node)
+    {
+        $depth = -2;


Can we add some info about this parameter?

mikadamczyk · 2018-09-11T11:12:17Z

eZ/Publish/Core/FieldType/Tests/RichText/Converter/Render/StyleTest.php

+    /**
+     * @var \eZ\Publish\Core\FieldType\RichText\RendererInterface|\PHPUnit\Framework\MockObject\MockObject
+     */
+    protected $rendererMock;


We place class attributes at the begging of the class

mikadamczyk · 2018-09-11T11:13:57Z

eZ/Publish/Core/MVC/Symfony/FieldType/RichText/Renderer.php

+        $this->customStylesConfiguration = $customStylesConfiguration;
+    }
+
+    public function renderStyle($name, array $parameters, $isInline)


missing docblock

mikadamczyk · 2018-09-11T11:21:06Z

eZ/Publish/Core/MVC/Symfony/FieldType/RichText/Renderer.php

+     */
+    protected function getStyleTemplateName($identifier, $isInline)
+    {
+        if (isset($this->customStylesConfiguration[$identifier]) && !empty($this->customStylesConfiguration[$identifier]['template'])) {


!empty($this->customStylesConfiguration[$identifier]['template']) will be enough if $customStylesConfiguration is initialize as array

eZ/Bundle/EzPublishCoreBundle/Resources/config/default_settings.yml

alongosz · 2018-09-11T11:38:30Z

eZ/Bundle/EzPublishCoreBundle/DependencyInjection/Configuration.php

+    {
+        return $ezRichTextNode
+                ->arrayNode('custom_styles')
+                // workaround: take into account Custom Tag names when merging configs


typo: Custom Style

alongosz · 2018-09-11T12:28:16Z

Note: it requires rebase, so while at it I'm gonna also address latest review comments to unblock our QA.

alongosz · 2018-09-11T13:10:47Z

Rebased
@mikadamczyk I resolved your (and my) requests via 3b0c151

Added a way to declare and use custom "styles" in richtext (https://alloyeditor.com/docs/features/styles.html)

…part of my text (#2427) * EZP-29554: Added Custom Styles support for RichText Field Type Added a way to declare and use custom "styles" in richtext (https://alloyeditor.com/docs/features/styles.html) * EZP-29554: Added improvements after code review

…part of my text This commit is a squash of the following ezpublish-kernel commits: - ezsystems/ezpublish-kernel@36f372c (ezsystems/ezpublish-kernel#2427) - ezsystems/ezpublish-kernel@143d9a6 (ezsystems/ezpublish-kernel#2457) * EZP-29554: Added Custom Styles support for RichText Field Type Added a way to declare and use custom "styles" in richtext (https://alloyeditor.com/docs/features/styles.html) * EZP-29664: Stored Custom Styles as eztemplate and eztemplateinline

erdnaxelaweb mentioned this pull request Aug 21, 2018

EZP-29554: As an editor, I want to be able to apply custom styles to part of my text ezsystems/ezplatform-admin-ui#602

Merged

2 tasks

crevillo reviewed Aug 21, 2018

View reviewed changes

alongosz changed the title ~~As an editor, i want to be able to apply custom styles to part of my text~~ EZP-29554: As an editor, I want to be able to apply custom styles to part of my text Aug 31, 2018

alongosz requested changes Aug 31, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

erdnaxelaweb force-pushed the feat-richtext-custom-styles branch from 2d7f38f to cf7a3a9 Compare September 9, 2018 18:29

alongosz approved these changes Sep 10, 2018

View reviewed changes

alongosz requested review from ViniTou, Nattfarinn, webhdx and mikadamczyk September 10, 2018 09:31

Nattfarinn approved these changes Sep 10, 2018

View reviewed changes

alongosz added the Ready for QA label Sep 10, 2018

micszo self-assigned this Sep 11, 2018

ViniTou approved these changes Sep 11, 2018

View reviewed changes

mikadamczyk requested changes Sep 11, 2018

View reviewed changes

alongosz reviewed Sep 11, 2018

View reviewed changes

alongosz force-pushed the feat-richtext-custom-styles branch from cf7a3a9 to 3b0c151 Compare September 11, 2018 13:08

mikadamczyk approved these changes Sep 11, 2018

View reviewed changes

Florian ALEXANDRE and others added 2 commits September 12, 2018 13:05

EZP-29554: Added Custom Styles support for RichText Field Type

0f0e701

Added a way to declare and use custom "styles" in richtext (https://alloyeditor.com/docs/features/styles.html)

EZP-29554: Added improvements after code review

a67c1fb

alongosz force-pushed the feat-richtext-custom-styles branch from 3b0c151 to a67c1fb Compare September 12, 2018 11:06

micszo approved these changes Sep 13, 2018

View reviewed changes

micszo added QA approved and removed Ready for QA labels Sep 13, 2018

micszo removed their assignment Sep 13, 2018

alongosz merged commit 36f372c into ezsystems:master Sep 13, 2018

alongosz mentioned this pull request Sep 13, 2018

EZP-59554: Created Online Editor Custom Styles documentation ibexa/documentation-developer#400

Merged

2 tasks

alongosz mentioned this pull request Oct 15, 2018

Applied changes from Kernel since extracting RichText ezsystems/ezplatform-richtext#7

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-29554: As an editor, I want to be able to apply custom styles to part of my text #2427

EZP-29554: As an editor, I want to be able to apply custom styles to part of my text #2427

erdnaxelaweb commented Aug 21, 2018 •

edited by alongosz

crevillo Aug 21, 2018

erdnaxelaweb Aug 22, 2018

alongosz Aug 31, 2018

alongosz left a comment

alongosz Aug 31, 2018

alongosz Aug 31, 2018

alongosz Aug 31, 2018

alongosz Aug 31, 2018

alongosz Aug 31, 2018

This comment has been minimized.

erdnaxelaweb commented Sep 9, 2018

ViniTou left a comment

mikadamczyk Sep 11, 2018

mikadamczyk Sep 11, 2018

mikadamczyk Sep 11, 2018

mikadamczyk Sep 11, 2018

alongosz Sep 11, 2018

alongosz commented Sep 11, 2018

alongosz commented Sep 11, 2018 •

edited

		@@ -0,0 +1 @@
		<div class="{% if align is defined %}align-{{ align }}{% endif %} style-{{ name }}">{% spaceless %}{{ content\|raw }}{% endspaceless %}</div>

		@@ -0,0 +1 @@
		<span class="style-{{ name }}">{% spaceless %}{{ content\|raw }}{% endspaceless %}</span>

EZP-29554: As an editor, I want to be able to apply custom styles to part of my text #2427

EZP-29554: As an editor, I want to be able to apply custom styles to part of my text #2427

Conversation

erdnaxelaweb commented Aug 21, 2018 • edited by alongosz

Configure the custom style

Default templates

Labels

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongosz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

erdnaxelaweb commented Sep 9, 2018

ViniTou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongosz commented Sep 11, 2018

alongosz commented Sep 11, 2018 • edited

erdnaxelaweb commented Aug 21, 2018 •

edited by alongosz

alongosz commented Sep 11, 2018 •

edited