refactor Elementor support (WP-854) by vsolovei-smartling · Pull Request #500 · Smartling/wordpress-localization-plugin

vsolovei-smartling · 2023-12-14T12:47:48Z

tests/IntegrationTests/testdata/elementor.json
tests/Smartling/ContentTypes/testMergeElementorData.json

…ements (WP-848)

add base helper methods and raw array for elements

vsolovei-smartling · 2023-12-14T13:01:46Z

+namespace Smartling\ContentTypes\Elementor;
+
+class ElementFactory {
+    public const UNKNOWN_ELEMENT = 'unknown';


This is rather the base element, this naming might be confusing, I'm open to propositions

I think we need two classes.

The first one is base class (maybe abstract). It just implements reusable methods (like getRelated)

Another one is Unknown, it will try to handle anything that was not parsed. I don't say that this class should be smart and extract what other classes missed. It can simply log - "I got this json .....". So initial responsibility will be just alerting us about an unknown element. Later you can add more logic that will try to send this json as is to smartling. But we need to brainstorm this.

Extracted abstract class

vsolovei-smartling · 2023-12-14T13:33:02Z

        return is_array($value) && 0 < count($value);
    }

+    public function flatten(array $array, string $base = '', string $divider = '/'): array


This is different from FieldFilterHelper, this preserves array elements types instead of converting them to string

vsolovei-smartling · 2023-12-14T13:40:08Z

+        $keys = ['background_image/alt', 'text'];
+        foreach ($this->elements as $element) {
+            if ($element instanceof Element) {
+                $return[] = $element->getTranslatableStrings();


Recursively search for strings in nested objects

vsolovei-smartling · 2023-12-14T13:40:41Z

+    {
+        foreach ($this->elements as &$element) {
+            if ($element instanceof Element) {
+                $element = $element->setTargetContent(


Recursively set relations and strings in nested objects

dimitrystd

I didn't finish the review.

dimitrystd · 2023-12-14T19:03:05Z

+namespace Smartling\ContentTypes\Elementor;
+
+class ElementFactory {
+    public const UNKNOWN_ELEMENT = 'unknown';


I think we need two classes.

The first one is base class (maybe abstract). It just implements reusable methods (like getRelated)

Another one is Unknown, it will try to handle anything that was not parsed. I don't say that this class should be smart and extract what other classes missed. It can simply log - "I got this json .....". So initial responsibility will be just alerting us about an unknown element. Later you can add more logic that will try to send this json as is to smartling. But we need to brainstorm this.

dimitrystd · 2023-12-15T23:50:27Z

+        return $return;
+    }
+
+    public function getTranslatableStrings(): array


I found an example of icon-list in your tests. Why these 4 fields are not mentioned here?

Those are from some extension, we'll need to handle them at some point of time, but first I'll find out which extension adds these properties, they're present for all objects, but the values everywhere I've seen so far are defaults

dimitrystd · 2023-12-16T00:00:17Z

+        foreach ($this->arrayHelper->flatten($info) as $item) {
+            assert($item instanceof Content);
+        }
+        $this->info = $info;


Instead of using the reference, I would recommend creating a new array. You cannot guarantee that the array passed as $info will not be modified by caller code.

No references are used here.
We ensure that every array element is Content, to avoid issues where __construct is called with an array of something else.
Further changes to $info will not affect the created RelatedContentInfo

dimitrystd · 2023-12-16T01:02:55Z

+        return $this->id;
+    }
+
+    public function getIntSettingByKey(string $key, array $settings): ?int


Mark all helper methods as protected. Maybe even make them static.

Made protected

review fixes and changes

superseded by \ElementorTest::testElementor

vsolovei-smartling added 10 commits December 7, 2023 19:20

add element interface, base element and factory (WP-848)

61db079

add text editor and heading widgets (WP-848)

ebe6840

add icon list and image widgets (WP-848)

1d3c0d7

add button, add getTranslatableStringsByKeys (WP-848)

e3834a9

add array merge with preserve keys, add common string keys for all el…

58bfb92

…ements (WP-848)

add related content models (WP-848)

37769e0

use related content, add icon and global widgets (WP-848)

7914912

add base helper methods and raw array for elements

add array helper flatten and structurize (WP-854)

decd6e7

add elementor related content processing (WP-854)

f4e2bad

add accordion widget support (WP-854)

1ba012b

vsolovei-smartling commented Dec 14, 2023

View reviewed changes

vsolovei-smartling requested review from anastasiia-kryshtop and dimitrystd December 14, 2023 14:25

dimitrystd reviewed Dec 14, 2023

View reviewed changes

cleanup (WP-854)

d029e4b

vsolovei-smartling changed the base branch from master to elementor-refactor December 15, 2023 14:05

dimitrystd reviewed Dec 16, 2023

View reviewed changes

vsolovei-smartling added 2 commits December 18, 2023 11:42

extract abstract class, fix associativity, cleanup (WP-854)

5590f65

review fixes and changes

move abstract class (WP-854)

6772518

vsolovei-smartling force-pushed the WP-854-elementor-2 branch from 8ec3e1a to 6772518 Compare December 18, 2023 11:00

vsolovei-smartling added 2 commits December 18, 2023 13:09

pull setRelations and setTargetContent up (WP-854)

a96ea86

remove test (WP-854)

d54076d

superseded by \ElementorTest::testElementor

vsolovei-smartling merged commit 126f1b1 into elementor-refactor Dec 18, 2023

vsolovei-smartling deleted the WP-854-elementor-2 branch December 18, 2023 12:46

vsolovei-smartling mentioned this pull request Dec 18, 2023

refactor elementor (WP-848) #499

Closed

Uh oh!

Conversation

vsolovei-smartling commented Dec 14, 2023 • edited by dimitrystd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimitrystd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vsolovei-smartling commented Dec 14, 2023 •

edited by dimitrystd

Loading