Add "Remove" and "Replace" overlay buttons + "Remove" and "Clone" sidebar buttons #3845

sobo-odoo · 2024-12-26T18:11:22Z

No description provided.

robodoo · 2024-12-26T18:11:25Z

This PR targets the un-managed branch odoo-dev/odoo:master-mysterious-egg, it needs to be retargeted before it can be merged.

robodoo · 2024-12-31T10:07:26Z

This PR targets the un-managed branch odoo-dev/odoo:master-mysterious-egg, it needs to be retargeted before it can be merged.

loco-odoo

just a few nitpick stuffs ;) Also, I did not check deeper but I was wondering if hasOverlayOptions should be adapted with isRemovable and isReplaceable ?

addons/html_builder/static/src/builder/plugins/remove/remove_plugin.js

loco-odoo · 2024-12-31T12:25:39Z

addons/html_builder/static/src/builder/plugins/remove/remove_plugin.js

+        const previousSiblingEl = this.getPreviousOrNextVisibleSibling("prev");
+        const nextSiblingEl = this.getPreviousOrNextVisibleSibling("next");


Maybe you want to add the condition on o_snippet_invisible as in stable (or add a todo to do it later)

This part will be done later 😉 (there is a TODO above)

addons/html_builder/static/src/builder/plugins/replace/replace_plugin.js

addons/html_builder/static/src/builder/plugins/remove/remove_plugin.js

addons/html_builder/static/src/builder/builder_sidebar/tabs/block_tab/block_tab.js

addons/html_builder/static/src/builder/plugins/overlay_buttons/overlay_buttons.xml

loco-odoo · 2024-12-31T13:19:11Z

addons/html_builder/static/tests/overlay_buttons.test.js

+    ).click();
+    // Check that the snippet was replaced by the chosen one.
+    expect(":iframe section.s_text_image").toHaveCount(0);
+    expect(":iframe section.s_shape_image").toHaveCount(1);


Maybe something that can be interesting to test as well is that the overlay is still here and the "customize" tab displays the correct info of the updated snippet

I am not sure to keep this behavior yet, I will add a TODO to not forget if I keep it 🙂

FrancoisGe · 2025-01-05T20:23:44Z

addons/html_builder/static/src/builder/plugins/remove/remove_plugin.js

+    const root = undefined;
+    return !unremovableNodePredicates.some((p) => p(el, root));


you don t need "root" ?

FrancoisGe

Good job :) Just small stuffs

FrancoisGe · 2025-01-05T20:28:11Z

addons/html_builder/static/src/builder/plugins/remove/remove_plugin.js

+    };
+
+    setup() {
+        this.target = null;


I m not a big fan to have the target on this.

FrancoisGe · 2025-01-05T20:30:38Z

addons/html_builder/static/src/builder/plugins/remove/remove_plugin.js

+    }
+
+    // TODO duplicated.
+    getPreviousOrNextVisibleSibling(direction) {


Maybe create an utils (target, direction). Rename getVisibleSibling ?

FrancoisGe · 2025-01-05T20:31:18Z

addons/html_builder/static/src/builder/plugins/remove/remove_plugin.js

+        const targetMobileOrder = this.target.style.order;
+        // On mobile, if the target has a mobile order (which is independent
+        // from desktop), consider these orders instead of the DOM order.
+        if (targetMobileOrder && this.isMobileView) {


isMobileView is not define

FrancoisGe · 2025-01-05T20:34:24Z

addons/html_builder/static/src/builder/plugins/remove/remove_plugin.js

+import { Plugin } from "@html_editor/plugin";
+import { resizeGrid } from "@html_builder/builder/utils/grid_layout_utils";
+import { unremovableNodePredicates as deletePluginPredicates } from "@html_editor/core/delete_plugin";
+import { isUnremovableQWebElement as qwebPluginPredicate } from "@html_editor/others/qweb_plugin";


You don t use isUnremovableQWebElement ?

FrancoisGe · 2025-01-05T20:36:35Z

addons/html_builder/static/tests/overlay_buttons.test.js

+            </div>
+        </section>
+    `,
+        { loadIframeBundles: true }


You need the style in your test ?

FrancoisGe · 2025-01-05T20:51:14Z

addons/html_builder/static/src/builder/plugins/replace/replace_plugin.js

+                callback: (snippet) => {
+                    newSnippet = snippet;
+                    resolve();


Why we need to pass a callback ?

FrancoisGe · 2025-01-05T20:51:55Z

addons/html_builder/static/src/builder/plugins/replace/replace_plugin.js

+    };
+
+    setup() {
+        this.target = null;


I m not a big fan to have the target on this

FrancoisGe · 2025-01-05T20:56:34Z

addons/html_builder/static/src/builder/builder_sidebar/tabs/block_tab/block_tab.js

    };

    setup() {
        this.dialog = useService("dialog");
        this.orm = useService("orm");
        this.company = useService("company");
+        this.onClickInstall = this.props.onInstallSnippetClick;


Not a good practice to put a props in local var. If the props change, it will not work. 2 solutions, replace each onClickInstall by props.installSnippetModule or use a getter

FrancoisGe · 2025-01-05T20:59:46Z

addons/html_builder/static/src/builder/builder_sidebar/builder_sidebar.js

+    removeElement(el) {
+        this.editor.shared.remove.removeElement(el);
+    }
+
+    cloneElement(el) {
+        this.editor.shared.clone.cloneElement(el);
+    }


CustomTab could directly use it ? You don t need to pass it as props or I miss something ?

FrancoisGe · 2025-01-05T21:03:52Z

addons/html_builder/static/src/builder/components/option_container.js

@@ -22,6 +26,37 @@ export class OptionsContainer extends Component {
    }

    get title() {
-        return this.env.getEditingElement().dataset.name;
+        const editingElement = this.env.getEditingElement();


Funny, i have a commit for this 7e563b8

sobo-odoo force-pushed the master-mysterious-egg-sobo branch 2 times, most recently from ca5091b to ca2410a Compare December 30, 2024 19:27

sobo-odoo marked this pull request as ready for review December 31, 2024 10:07

loco-odoo reviewed Dec 31, 2024

View reviewed changes

sobo-odoo force-pushed the master-mysterious-egg-sobo branch 3 times, most recently from 00640b7 to 578176b Compare January 3, 2025 11:01

sobo-odoo added 5 commits January 3, 2025 20:16

Add "Remove" overlay button

d9713e9

Add test for "Remove" overlay button

7f3d884

Add "Replace" overlay button

e2e6ff9

Add test for "Replace" overlay button

28f264d

Small details fixes

2b5fff8

sobo-odoo force-pushed the master-mysterious-egg-sobo branch from 578176b to fe46e95 Compare January 3, 2025 19:19

sobo-odoo added 2 commits January 3, 2025 20:34

Add "Remove" and "Clone" sidebar buttons (options containers)

1eade90

Add tests for "Remove" and "Clone" sidebar buttons

02a18f4

sobo-odoo force-pushed the master-mysterious-egg-sobo branch from fe46e95 to 02a18f4 Compare January 3, 2025 19:36

sobo-odoo changed the title ~~Add "Remove" overlay button~~ Add "Remove" and "Replace" overlay buttons + "Remove" and "Clone" sidebar buttons Jan 3, 2025

FrancoisGe reviewed Jan 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Remove" and "Replace" overlay buttons + "Remove" and "Clone" sidebar buttons #3845

Add "Remove" and "Replace" overlay buttons + "Remove" and "Clone" sidebar buttons #3845

sobo-odoo commented Dec 26, 2024

robodoo commented Dec 26, 2024

robodoo commented Dec 31, 2024

loco-odoo left a comment

loco-odoo Dec 31, 2024

sobo-odoo Dec 31, 2024

loco-odoo Dec 31, 2024

sobo-odoo Dec 31, 2024

FrancoisGe Jan 5, 2025

FrancoisGe left a comment

FrancoisGe Jan 5, 2025

FrancoisGe Jan 5, 2025

FrancoisGe Jan 5, 2025

FrancoisGe Jan 5, 2025

FrancoisGe Jan 5, 2025

FrancoisGe Jan 5, 2025

FrancoisGe Jan 5, 2025

FrancoisGe Jan 5, 2025

FrancoisGe Jan 5, 2025

FrancoisGe Jan 5, 2025

		const previousSiblingEl = this.getPreviousOrNextVisibleSibling("prev");
		const nextSiblingEl = this.getPreviousOrNextVisibleSibling("next");

		const root = undefined;
		return !unremovableNodePredicates.some((p) => p(el, root));

Add "Remove" and "Replace" overlay buttons + "Remove" and "Clone" sidebar buttons #3845

Are you sure you want to change the base?

Add "Remove" and "Replace" overlay buttons + "Remove" and "Clone" sidebar buttons #3845

Conversation

sobo-odoo commented Dec 26, 2024

robodoo commented Dec 26, 2024

robodoo commented Dec 31, 2024

loco-odoo 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

FrancoisGe 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

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