Replace text angular editor with trix editor in About Us and Shopfront message fields #12734

cillian · 2024-08-02T16:26:24Z

What? Why?

For [Bye Bye Angular] Enterprise Settings -> 'About' #9481 and [Bye Bye Angular] Enterprise Settings -> 'Shop Preferences' #9490

The trix editor was added in #11140, this PR follows on from that and replaces the remaining instances of the text-angular editor.

What should we test?

Test the Trix editor is working for the following fields...
- Enterprise Settings > About > About us
- Enterprise Settings > Shop preferences > Shopfront message
- Enterprise Settings > Shop preferences > Shopfront closed message
- Enterprise Group Settings > About > About

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

Before

After

Note, in the above screenshots the trix fields are on new lines instead of being alongside the field label like they usually are. This is because there was a horizontal scrollbar if it was beside the field label, see:

I also added some wrapping to CSS to the trix editor button groups so they will automatically wrap and stop the horizontal scrollbar on smaller screens, although another solution might be to remove the whitespace to the right, see:

…ges and about fields for enterprises and enterprise groups

app/models/enterprise.rb

cillian · 2024-08-09T09:53:06Z

app/webpacker/css/admin/trix.scss

@@ -1,3 +1,7 @@
+trix-toolbar .trix-button-row {
+  flex-wrap: wrap;


Prevents horizontal scrollber on Trix button toolbar in smaller screens.

rioug

yeah less angular 🧹 ! Thanks @cillian 🙏

app/models/enterprise.rb

dacook

Great work, it looks much cleaner than before!
I like how you adapted to fit the extra Trix buttons in, and it means there's more space for the text area too.

I have a couple of comments that might be worth trying. Do they make sense?
If not, we can proceed as is.

app/models/enterprise.rb

app/views/admin/enterprise_groups/_form_about.html.haml

dacook · 2024-08-12T07:24:02Z

config/locales/en.yml

+    trix:
+      attachFiles: "Attach Files"
+      bold: "Bold"
+      bullets: "Bullets"
+      byte: "Byte"
+      bytes: "Bytes"
+      captionPlaceholder: "Add a caption…"
+      code: "Code"
+      heading1: "Heading"
+      indent: "Increase Level"
+      italic: "Italic"
+      link: "Link"
+      numbers: "Numbers"
+      outdent: "Decrease Level"
+      quote: "Quote"
+      redo: "Redo"
+      remove: "Remove"
+      strike: "Strikethrough"
+      undo: "Undo"
+      unlink: "Unlink"
+      url: "URL"
+      urlPlaceholder: "Enter a URL…"
+      GB: "GB"
+      KB: "KB"
+      MB: "MB"
+      PB: "PB"
+      TB: "TB"


Thanks for adding Trix translation keys.

These two old labels seem equivalent to new labels (I haven't checked though)

"Insert / edit link" -> "Link"

"Please enter a URL to insert" -> "Enter a URL…"

If so, I think it would help to use the old labels. That way, it will probably be easy for translators to copy the existing labels when adding translations for Trix. Does that make sense?

Good idea, that's updated in 2d6ffc0 to use the old "Please enter a URL to insert" label.

The "Link" translation is used in two places at once so the "Insert / edit link" label is a bit more awkward, see...

…he Trix editor

… in Chrome

…ses_spec.rb

… set automatically

cillian · 2024-08-16T15:55:00Z

app/webpacker/controllers/trixeditor_controller.js

+        Trix.config.lang[key] = translation;
+
+        // Set toolbar translations (Chrome)
+        this.#setTranslation(
+          `[data-trix-action="${this.#translationKeyToAttributeMappings(key)}"]`,
+          "title",
+          translation
+        );
+        this.#setTranslation(
+          `[data-trix-attribute="${this.#translationKeyToAttributeMappings(key)}"]`,
+          "title",
+          translation
+        );
+      }
+
+      // Set translations for link dialog (Chrome)
+      this.#setTranslation(`[data-trix-dialog="href"] input`,
+                                  "aria-label", I18n.t("js.trix.url"));
+      this.#setTranslation(`[data-trix-dialog="href"] input`,
+                                  "placeholder", I18n.t("js.trix.urlPlaceholder"));
+      this.#setTranslation('.trix-dialog--link input[data-trix-method="setAttribute"]',
+                                  "value", I18n.t("js.trix.link"));
+      this.#setTranslation('.trix-dialog--link input[data-trix-method="removeAttribute"]',
+                                  "value", I18n.t("js.trix.unlink"));


Trix.config.lang[key] = translation; only seems to work in Firefox for some reason. The translations have to be set more manually for Chrome.

That's interesting, and unfortunate because you have map quite a lot data.

According to this wiki, it suggests overwriting the lang config in one command.

Trix.config.lang = translation
https://github.com/basecamp/trix/wiki/I18n

But you tried that and it didn't work, so it would be worth raising an issue.
I had a look for issues and could only find one.
The final comment shows another method using Object.assign. I don't know if that would make a different, but perhaps it's worth trying.
basecamp/trix#874 (comment)

Good find, fixed by 63c62ca and it works in Firefox and Chrome.

dacook · 2024-08-19T05:33:51Z

config/locales/en_AU.yml

+    trix:
+      bold: "Bold"
+      bullets: "Bullets"
+      code: "Code"
+      heading1: "Heading"
+      hr: "Horizontal rule"
+      indent: "Increase Level"
+      italic: "Italic"
+      link: "Link"
+      numbers: "Numbers"
+      outdent: "Decrease Level"
+      quote: "Quote"
+      redo: "Redo"
+      strike: "Strikethrough"
+      undo: "Undo"
+      unlink: "Unlink"
+      url: "URL"
+      urlPlaceholder: "Please enter a URL to insert"


Actually, these will already have the default loaded from en.yml

In general , we need to avoid changing any locale files other than the default, as described here:
https://github.com/openfoodfoundation/openfoodnetwork/wiki/Internationalisation-(i18n)#development

I have to admit, I don't know if this commit will be a problem or not, but I know it's not necessary so perhaps it's safest to remove it.

dacook · 2024-08-19T05:47:37Z

app/webpacker/controllers/trixeditor_controller.js

+        Trix.config.lang[key] = translation;
+
+        // Set toolbar translations (Chrome)
+        this.#setTranslation(
+          `[data-trix-action="${this.#translationKeyToAttributeMappings(key)}"]`,
+          "title",
+          translation
+        );
+        this.#setTranslation(
+          `[data-trix-attribute="${this.#translationKeyToAttributeMappings(key)}"]`,
+          "title",
+          translation
+        );
+      }
+
+      // Set translations for link dialog (Chrome)
+      this.#setTranslation(`[data-trix-dialog="href"] input`,
+                                  "aria-label", I18n.t("js.trix.url"));
+      this.#setTranslation(`[data-trix-dialog="href"] input`,
+                                  "placeholder", I18n.t("js.trix.urlPlaceholder"));
+      this.#setTranslation('.trix-dialog--link input[data-trix-method="setAttribute"]',
+                                  "value", I18n.t("js.trix.link"));
+      this.#setTranslation('.trix-dialog--link input[data-trix-method="removeAttribute"]',
+                                  "value", I18n.t("js.trix.unlink"));


That's interesting, and unfortunate because you have map quite a lot data.

According to this wiki, it suggests overwriting the lang config in one command.

Trix.config.lang = translation
https://github.com/basecamp/trix/wiki/I18n

But you tried that and it didn't work, so it would be worth raising an issue.
I had a look for issues and could only find one.
The final comment shows another method using Object.assign. I don't know if that would make a different, but perhaps it's worth trying.
basecamp/trix#874 (comment)

dacook · 2024-08-19T05:50:49Z

config/locales/ar.yml

+    trix:
+      urlPlaceholder: "الرجاء إدخال عنوان URL لإدراجه"


Sorry I just meant to update it in en.yml. In general we should avoid changing other locale files directly. I'm unsure if adding new keys is a problem though.

@mkllnk can you help with a query here?
For convenience, Cillian has copied an existing translation to a new key (the existing translation was for angular editor, and there is an equivalent in the new trix editor). Do you know if it's ok, and if it's helpful to add new keys to the locale files?
Or should we remove this before merging?

We should remove this addition. Transifex will handle it. It recognises existing translations.

mkllnk

Great work! Thank you.

We just need to revert he changes to the other locales. Otherwise this is great!

mkllnk · 2024-08-20T02:50:47Z

config/locales/ar.yml

-          shopfront_message_link_tooltip: "إدراج / تحرير الرابط"
-          shopfront_message_link_prompt: "الرجاء إدخال عنوان URL لإدراجه"


Please remove all changes to locales other than en.yml.

👍 Fixed by dd51755 and 9895116

mkllnk · 2024-08-20T02:53:47Z

config/locales/ar.yml

+    trix:
+      urlPlaceholder: "الرجاء إدخال عنوان URL لإدراجه"


We should remove this addition. Transifex will handle it. It recognises existing translations.

Co-authored-by: David Cook <[email protected]>

…tor in the Trix editor" for all locales except :en This reverts commit 2d6ffc0.

…gions" This reverts commit 70ca031.

cillian · 2024-08-23T09:31:50Z

Thanks. I have reverted those changes to non en locale files now and simplified how the Trix translations are set, let me know if you want me to squash the commits.

dacook

Great, all ready to test!

drummer83 · 2024-08-29T16:33:34Z

@cillian Github is saying there are changes requested. Could you please have a look? I assume it's been addressed already?

mkllnk

Github is saying there are changes requested.

Github was waiting for my review. But the other approvals should have been enough. Anyway, you get may approval, too. 😄

drummer83 · 2024-09-27T07:14:48Z

Hi @cillian
Sorry, there's a conflict here. Could you please have a look?
Thanks!

drummer83 · 2024-09-27T07:35:12Z

For future testing I have documented the "before" state already.

Already existing Trix editor for White Label Custom Tab Content:
Same on narrow screens (scroll bar and still truncated buttons):
Old editor for Home and Shopfront Closed message with test content using all available options:
What it looks like in the shopfront:

cillian · 2024-09-27T09:32:21Z

Thanks @drummer83 I have fixed that conflict now.

For reference, it was this...

  # Remove any unsupported HTML.
<<<<<<< HEAD
  def long_description
    HtmlSanitizer.sanitize_and_enforce_link_target_blank(super)
  end

  # Remove any unsupported HTML.
=======
>>>>>>> master
  def long_description=(html)
    super(HtmlSanitizer.sanitize_and_enforce_link_target_blank(html))
  end

To fix it I removed the #long_description method like what was done in d061fe8 because the enforcing of target="_blank" will be added to links when assigning the long_description= attribute. Before this PR all links already had target="_blank" on them because that was the default behaviour of the old angular text editor.

drummer83 · 2024-09-29T15:09:51Z

Hi @cillian,
Thanks for working on this one! Here are my test results after staging.

After staging the PR

Shopfront display remains unchanged.
Back office editor is updated and styling is adapted to newly available options (One header only, no underline, ...).

After staging the PR and updating the content

Shopfront display is adapted to use newly available options.
Behavior on narrow screens is much better now.

I find it surprising to see, that it's enough to edit any of the content to have the new styles applied to all of the occurences. It's no problem, maybe even an advantage looking at consistency. I assume this happens because when clicking Update, all of the tabs are saved and not just the tab I'm looking at.

Summary

Styling changes in the shop front due to different styling options in the new editor.
This is not critical, because all information is still available.
Small screens are much better supported.
No problems found.

We can merge this one, but the branch is out of date and I can't rebase due to a conflict. I will move this to Ready To Go but add the feedback needed label to have someone else have a look at it. Maybe @filipefurtad0 or @sigmundpetersen could have a look? 🙏

Thanks again Cillian! 🚀

sigmundpetersen · 2024-09-29T15:52:18Z

Merging is blocked because reapproval is needed after Cillian resolved the last conflict.
Maybe @mkllnk or @rioug could have a quick look when coming back to work and merge if everything looks Ok 👍

github-actions bot assigned cillian Aug 2, 2024

cillian force-pushed the replace-text-angular-with-trix branch 2 times, most recently from b0567f7 to 837e187 Compare August 9, 2024 08:51

Replace text-angular editor with trix editor in fields for shop messa…

a6d3909

…ges and about fields for enterprises and enterprise groups

cillian force-pushed the replace-text-angular-with-trix branch from 837e187 to a6d3909 Compare August 9, 2024 09:37

cillian marked this pull request as ready for review August 9, 2024 09:44

cillian commented Aug 9, 2024

View reviewed changes

app/models/enterprise.rb Outdated Show resolved Hide resolved

cillian commented Aug 9, 2024

View reviewed changes

rioug self-requested a review August 12, 2024 01:16

rioug added the user facing changes Thes pull requests affect the user experience label Aug 12, 2024

rioug approved these changes Aug 12, 2024

View reviewed changes

app/models/enterprise.rb Outdated Show resolved Hide resolved

dacook reviewed Aug 12, 2024

View reviewed changes

cillian added 7 commits August 16, 2024 15:31

Reuse 'Please insert a URL' translation from text angular editor in t…

2d6ffc0

…he Trix editor

Don't add translations for Trix buttons and functions we are not using

7961ff7

Add Trix translations for all the different English locale regions

70ca031

Fix issue where Trix editor translations were not being set correctly…

429e2b0

… in Chrome

Fix expected placeholder in test after updating translation

a2c3ac2

Fix Layout/LineLength Rubocop violation in spec/system/admin/enterpri…

b174080

…ses_spec.rb

Don't need to set value on hidden fields for Trix editors, it will be…

ae2e92f

… set automatically

cillian commented Aug 16, 2024

View reviewed changes

Rename method for looking up trix attribute or action by translation key

08ab405

dacook reviewed Aug 19, 2024

View reviewed changes

dacook requested a review from mkllnk August 20, 2024 01:46

mkllnk requested changes Aug 20, 2024

View reviewed changes

cillian and others added 5 commits August 23, 2024 09:47

Simplify setting of Trix editor translations

63c62ca

Co-authored-by: David Cook <[email protected]>

Revert "Reuse 'Please insert a URL' translation from text angular edi…

a745249

…tor in the Trix editor" for all locales except :en This reverts commit 2d6ffc0.

Revert "Add Trix translations for all the different English locale re…

9895116

…gions" This reverts commit 70ca031.

Don't make any changes to non :en locales.

dd51755

Revert whitespace changes in config/locales/en.yml

60afa4d

sigmundpetersen requested review from mkllnk and dacook August 23, 2024 09:48

rioug approved these changes Aug 26, 2024

View reviewed changes

dacook approved these changes Aug 26, 2024

View reviewed changes

drummer83 added the feedback-needed label Aug 29, 2024

mkllnk approved these changes Aug 29, 2024

View reviewed changes

mkllnk removed the feedback-needed label Aug 29, 2024

drummer83 self-assigned this Sep 21, 2024

drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Sep 27, 2024

drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Sep 27, 2024

drummer83 removed their assignment Sep 27, 2024

drummer83 added the feedback-needed label Sep 27, 2024

Merge in latest :master and resolve conflict in app/models/enterprise.rb

f474afa

sigmundpetersen removed the feedback-needed label Sep 27, 2024

drummer83 self-assigned this Sep 29, 2024

drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Sep 29, 2024

drummer83 added feedback-needed and removed pr-staged-au staging.openfoodnetwork.org.au labels Sep 29, 2024

rioug approved these changes Sep 29, 2024

View reviewed changes

rioug removed the feedback-needed label Sep 29, 2024

rioug merged commit 91f2ca9 into openfoodfoundation:master Sep 29, 2024
55 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace text angular editor with trix editor in About Us and Shopfront message fields #12734

Replace text angular editor with trix editor in About Us and Shopfront message fields #12734

cillian commented Aug 2, 2024 •

edited

Loading

cillian Aug 9, 2024 •

edited

Loading

rioug left a comment

dacook left a comment

dacook Aug 12, 2024

cillian Aug 16, 2024

cillian Aug 16, 2024

dacook Aug 19, 2024

cillian Aug 23, 2024 •

edited

Loading

dacook Aug 19, 2024

dacook Aug 19, 2024

dacook Aug 19, 2024

dacook Aug 19, 2024

mkllnk Aug 20, 2024

mkllnk left a comment

mkllnk Aug 20, 2024

cillian Aug 23, 2024 •

edited

Loading

mkllnk Aug 20, 2024

cillian commented Aug 23, 2024

dacook left a comment

drummer83 commented Aug 29, 2024

mkllnk left a comment

drummer83 commented Sep 27, 2024

drummer83 commented Sep 27, 2024

cillian commented Sep 27, 2024

drummer83 commented Sep 29, 2024

sigmundpetersen commented Sep 29, 2024

		@@ -1,3 +1,7 @@
		trix-toolbar .trix-button-row {
		flex-wrap: wrap;

		trix:
		urlPlaceholder: "الرجاء إدخال عنوان URL لإدراجه"

		shopfront_message_link_tooltip: "إدراج / تحرير الرابط"
		shopfront_message_link_prompt: "الرجاء إدخال عنوان URL لإدراجه"

Replace text angular editor with trix editor in About Us and Shopfront message fields #12734

Replace text angular editor with trix editor in About Us and Shopfront message fields #12734

Conversation

cillian commented Aug 2, 2024 • edited Loading

What? Why?

What should we test?

Release notes

cillian Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

rioug left a comment

Choose a reason for hiding this comment

dacook 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

cillian Aug 23, 2024 • edited Loading

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

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cillian Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cillian commented Aug 23, 2024

dacook left a comment

Choose a reason for hiding this comment

drummer83 commented Aug 29, 2024

mkllnk left a comment

Choose a reason for hiding this comment

drummer83 commented Sep 27, 2024

drummer83 commented Sep 27, 2024

cillian commented Sep 27, 2024

drummer83 commented Sep 29, 2024

After staging the PR

After staging the PR and updating the content

Summary

sigmundpetersen commented Sep 29, 2024

cillian commented Aug 2, 2024 •

edited

Loading

cillian Aug 9, 2024 •

edited

Loading

cillian Aug 23, 2024 •

edited

Loading

cillian Aug 23, 2024 •

edited

Loading