The Rotate effect UI does not accept to leave the background color blank (=transparent). Probably this was introduced in #2251223: ConfigurableImageEffectInterface should use ConfigurablePluginInterface and PluginFormInterface.

CommentFileSizeAuthor
#7 2307053-7.patch1.72 KBmondrake
#4 2307053-4.patch1.72 KBmondrake
#1 2307053-1.patch1.87 KBmondrake
#1 2307053-1-test-only.patch930 bytesmondrake

Comments

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Rotate effect does not allow to set background to transparent » Rotate effect UI does not allow to set background color to transparent
Status: Active » Needs review
StatusFileSize
new 2307053-1-test-only.patch930 bytes
new 2307053-1.patch1.87 KB

Test + fix. Test only patch should fail, full patch pass.

The last submitted patch, 1: 2307053-1-test-only.patch, failed testing.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented
Status: Needs review » Needs work

1 small thing in the patch:

+++ b/core/modules/image/src/Plugin/ImageEffect/RotateImageEffect.php
@@ -127,8 +127,8 @@ public function buildConfigurationForm(array $form, array &$form_state) {
+      form_set_error('bgcolor', $form_state, $this->t('Background color must be a hexadecimal color value, or blank for transparency on image types that support it.'));
     }

I am not sure that we should repeat the whole help text in the error message. I find the existing message better (the user has filled in something, so probably wants to define a color).

And another small suggestion, related to this issue, as it is also in the add/edit rotate effect form UI:
On testing the add/edit rotate effect in the UI, I saw that the & deg ; html encoding was escaped. I would like to suggest to directly use the utf-8 character for degrees instead of the html escape sequence. The same for the twig template. So just add the lines below to the patch and that is solved as well:

diff --git a/core/modules/image/src/Plugin/ImageEffect/RotateImageEffect.php b/core/modules/image/src/Plugin/ImageEffect/RotateImageEffect.php
index d308266..c0cb5e8 100644
--- a/core/modules/image/src/Plugin/ImageEffect/RotateImageEffect.php
+++ b/core/modules/image/src/Plugin/ImageEffect/RotateImageEffect.php
@@ -103,7 +103,7 @@ public function buildConfigurationForm(array $form, array &$form_state) {
       '#default_value' => $this->configuration['degrees'],
       '#title' => t('Rotation angle'),
       '#description' => t('The number of degrees the image should be rotated. Positive numbers are clockwise, negative are counter-clockwise.'),
-      '#field_suffix' => '°',
+      '#field_suffix' => '°',
       '#required' => TRUE,
     );
     $form['bgcolor'] = array(
diff --git a/core/modules/image/templates/image-rotate-summary.html.twig b/core/modules/image/templates/image-rotate-summary.html.twig
index c3cae9d..705a0eb 100644
--- a/core/modules/image/templates/image-rotate-summary.html.twig
+++ b/core/modules/image/templates/image-rotate-summary.html.twig
@@ -20,8 +20,8 @@
 #}
 {% if data.random %}
   {% trans %}
-    random between -{{ data.degrees|abs }}° and {{ data.degrees|abs }}°
+    random between -{{ data.degrees|abs }}° and {{ data.degrees|abs }}°
   {% endtrans %}
 {% else %}
-  {{ data.degrees }}°
+  {{ data.degrees }}°
 {% endif %}

I think this is easier and faster then a new separate issue.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new 2307053-4.patch1.72 KB

I am not sure that we should repeat the whole help text in the error message. I find the existing message better (the user has filled in something, so probably wants to define a color).

Fine, let's just fix the bug here. Patch updated & rerolled.

On testing the add/edit rotate effect in the UI, I saw that the & deg ; html encoding was escaped

This is due to double escaping introduced by #1825952: Turn on twig autoescape by default, it was working right before that being committed. See also #2297711: Fix HTML escaping due to Twig autoescape. We need to see if #1938910: Convert image theme tables to table #type would solve that, or open a new issue specifically for this. I wouldn't address that here.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented
Status: Needs review » Reviewed & tested by the community

Follow-up: #2318297: Rotate effect: do not use html entity for 'degree' but its unicode character.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2307053-4.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
StatusFileSize
new 2307053-7.patch1.72 KB

Rerolled after #2316533: Add getValue/setValue/hasValue and isValueEmpty to FormState

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Patch (to be ported)

Back to RTBC

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Patch (to be ported) » Reviewed & tested by the community

Ops...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed cdab062 and pushed to 8.0.x. Thanks!

  • alexpott committed cdab062 on 8.0.x 
    Issue #2307053 by mondrake: Fixed Rotate effect UI does not allow to set...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.