Builder overlay + Sizing #3501

sobo-odoo · 2024-11-12T17:57:23Z

No description provided.

robodoo · 2024-11-12T17:57:26Z

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-11-15T14:05:44Z

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

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay.js

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay_plugin.js

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

FrancoisGe · 2024-11-15T16:37:27Z

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay_plugin.js

=== _update ?

I would say that, for now, let's keep them separated: when the sizing,... will be implemented, we could still add an update function later to do both at once if we repeat them a lot 🙂

And if you meant "using the _update function of the plugin after calling toggleOverlay", then I prefer doing the refresh here, so the overlay logic is grouped at one place (easier to follow the flow), like each overlay updates itself 👍

FrancoisGe · 2024-11-15T16:38:11Z

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay_plugin.js

do we still need need with _update

I prefer keeping everything separated, because we do not always need to update both at the same time, e.g.:

scrolling only needs to refresh the position, as the handles are not supposed to change

using an option may change both the size and the "status" of an element, so we need to refresh both the handles and the position

However, I agree that refreshHandles alone is not needed yet, but let's keep it for now to be consistent, we can remove it later if really no case needs it in the end 🙂

Also, the update was needed to use it in handleCommand, and also to make all the changes in one loop, to be a bit more efficient 🙂

FrancoisGe · 2024-11-15T16:38:50Z

addons/web/static/src/core/utils/scrolling.js

window.document === document ?

rename contextItem => scrollingElement

document is not defined there, so we need to use window.document 🙂 (see the function just above that uses it too)

sobo-odoo · 2024-11-15T19:16:35Z

I made the changes, see diff 🙂 (don't mind the NumberInput.js file that you already fixed, I rebased after my changes 😉)

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay.scss

Goaman · 2024-11-20T13:31:26Z

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay_plugin.js

+
+        // On keydown, hide the overlay and then show it again when the mouse
+        // moves.
+        let wasKeydown;


I would rather have a variable isOverlayVisible set by toggleOverlayVisibility. If we try to set a visibility that is the same as the previous one, we directly return.

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay_plugin.js

Goaman · 2024-11-20T13:51:53Z

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay_plugin.js

+        // TODO check if resizeObserver still needed.
+        // this.resizeObserver = new ResizeObserver(this.update.bind(this));
+        // this.resizeObserver.observe(this.overlayTarget);


I would still add it if some code changes the size of the box without adding a step (eg. a preview)

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay_plugin.js

Goaman · 2024-11-20T14:11:07Z

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay_plugin.js

+    refreshPosition() {
+        this.overlays.forEach((overlay) => {
+            overlay.refreshPosition();
+        });
+    }
+
+    refreshHandles() {
+        this.overlays.forEach((overlay) => {
+            overlay.refreshHandles();
+        });
+    }


As refreshing the handles is rather cheap, I would call update when we need to refresh the positions to avoid having one more method.

refreshHandles is not call in the plugin so we can remove the method.

Goaman · 2024-11-20T14:11:28Z

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay_plugin.js

+        });
+    }
+
+    toggleOverlayVisibility(show) {


setOverlaysVisibility?

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay_plugin.js

fdardenne

Well play ! This is a huge feature 💯
A few of my comments are nitpicking, you can ignore few of those if you want 👍

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay.js

addons/html_builder/static/src/builder/utils/grid_layout_utils.js

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay.js

addons/html_builder/static/src/builder/utils/grid_layout_utils.js

Needed in order for the sizing to be smoother (compared to master) but mostly to allow it to be tested, as the tests make big pointer moves, which therefore did not work with the previous implementation since it increments the indexes one by one, instead of considering the delta.

sobo-odoo force-pushed the master-mysterious-egg-sobo branch 3 times, most recently from 7417088 to cdc50b5 Compare November 14, 2024 15:42

sobo-odoo marked this pull request as ready for review November 15, 2024 14:05

FrancoisGe force-pushed the master-mysterious-egg branch from aeacbc1 to 395880e Compare November 15, 2024 15:21

sobo-odoo force-pushed the master-mysterious-egg-sobo branch from 57ca2df to e5a0389 Compare November 15, 2024 16:05

FrancoisGe reviewed Nov 15, 2024

View reviewed changes

sobo-odoo force-pushed the master-mysterious-egg-sobo branch 2 times, most recently from 318f42b to 2b89573 Compare November 15, 2024 19:14

Goaman reviewed Nov 20, 2024

View reviewed changes

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay.scss Outdated Show resolved Hide resolved

Goaman reviewed Nov 20, 2024

View reviewed changes

addons/html_builder/static/src/builder/plugins/builder_overlay/builder_overlay.scss Outdated Show resolved Hide resolved

Goaman reviewed Nov 20, 2024

View reviewed changes

sobo-odoo force-pushed the master-mysterious-egg-sobo branch 4 times, most recently from 5d420ac to 2071836 Compare November 28, 2024 10:30

sobo-odoo force-pushed the master-mysterious-egg-sobo branch 4 times, most recently from 53d83ba to 7b6623f Compare December 3, 2024 17:14

sobo-odoo force-pushed the master-mysterious-egg-sobo branch 3 times, most recently from fef1800 to e3afa34 Compare December 10, 2024 13:04

sobo-odoo changed the title ~~Builder overlay~~ Builder overlay + Sizing Dec 10, 2024

fdardenne reviewed Dec 10, 2024

View reviewed changes

[FIX] add an overlay for every options element

6d63e14

sobo-odoo added 6 commits December 10, 2024 17:58

Add simple builder overlay test

54c860c

Convert sizing options

285d865

Add tests for sizing

9b0f8a1

Use html_builder grid layout utils

6c2a7b0

Remove some props warning

9ff074b

sobo-odoo force-pushed the master-mysterious-egg-sobo branch from f2ad721 to 9ff074b Compare December 10, 2024 17:09

fdardenne merged commit 41444b2 into master-mysterious-egg Dec 11, 2024

fdardenne deleted the master-mysterious-egg-sobo branch December 11, 2024 07:03

Uh oh!

Builder overlay + Sizing #3501

Builder overlay + Sizing #3501

Uh oh!

Conversation

sobo-odoo commented Nov 12, 2024

Uh oh!

robodoo commented Nov 12, 2024

Uh oh!

robodoo commented Nov 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

sobo-odoo Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sobo-odoo commented Nov 15, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

fdardenne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sobo-odoo Nov 15, 2024 •

edited

Loading