[ADD] html_builder: convert s_google_map #4242

Zynton · 2025-03-24T08:54:52Z

Conversion of the Google Maps ("s_google_map") snippet.

robodoo · 2025-03-24T08:54:54Z

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

Steps to reproduce: - Activate developer mode - Install website Google Maps module - Navigate to Website in edit mode - Drop the google maps snippet - Enter a correct API key - The dialog closes but the snippet is removed This is due to a promise not resolved when confirming the API key.

robodoo · 2025-04-01T13:17:58Z

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

Zynton

Ready for review!
I left a few comments in review for whoever will review this. This was not a straight forward conversion. I tried to preserve the original implementation in general where possible. Nothing should be missing but it's hard to guarantee given the number of pieces interacting for this in the original code.

Zynton · 2025-04-01T13:19:37Z

addons/website/static/src/js/editor/snippets.editor.js

@@ -273,8 +273,7 @@ export class WebsiteSnippetsMenu extends weSnippetEditor.SnippetsMenu {
                onMounted(() => this.props.onMounted(this.modalRef));


This commit comes from this PR.

Zynton · 2025-04-01T13:20:14Z

addons/html_builder/static/src/plugins/google_map_option/google_map_option_plugin.js

+        // @TODO mysterious-egg: the `google_map service` is a duplicate of the
+        // `website_map_service`, but without the dependency on public
+        // interactions. These are used only to restart the interactions once
+        // the API is loaded. We do this in the plugin instead. Once
+        // `html_builder` replaces `website`, we should be able to remove
+        // `website_map_service` since only google_map service will be used.


@todo mysterious-egg

Zynton · 2025-04-01T13:20:23Z

addons/html_builder/static/src/plugins/google_map_option/google_map_option_plugin.js

+            // @TODO mysterious-egg: should we still do this, which was just
+            // done as a result of reporting a critical error?


@todo mysterious-egg

Zynton · 2025-04-01T13:22:30Z

addons/html_builder/static/src/plugins/google_map_option/google_map_option.scss

+            cursor: pointer;
+        }
+
+        /* Remove Google Map's own icon. */


The whole file is just a duplicate from the original snippet, except for this rule. In the current website builder, Google Maps' Autocomplete doesn't inject its style into the document (probably because it does so in the iframe instead) so we style from a blank slate. Here however we need to revert the icon styles so we don't end up with two icons.

It s maybe because we don t use the custom css. We try to use bootstrap if it s possible. The website builder sidebar style is not working correctly. A designer will probably try to make it better and use bootstrap

Are you saying I shouldn't bring in this css and I should try to do all this with bootstrap instead?

No wait, what am I saying, that's not possible, this is not in the sidebar, it's Google stuff in position absolute in a div somewhere in the body.

Zynton · 2025-04-01T13:23:19Z

addons/html_builder/static/src/plugins/google_map_option/google_map_option_plugin.js

@@ -27,7 +27,7 @@ import { GoogleMapOption } from "./google_map_option";
 * @typedef {{ isValid: boolean, message?: string }} ApiKeyValidation
 */

-class GoogleMapOptionPlugin extends Plugin {
+export class GoogleMapOptionPlugin extends Plugin {


Here are a number of small changes brought to make it easier to test.

Zynton · 2025-04-01T13:25:25Z

addons/html_builder/static/src/plugins/google_maps_option/google_maps_api_key_dialog.js

@@ -7,11 +7,11 @@ import { Component, useState, useRef } from "@odoo/owl";
 * @typedef {import('./google_map_option_plugin.js').ApiKeyValidation} ApiKeyValidation


I made this commit separately to be able to revert it, in case. But it drives me crazy that in most places Google Maps was names Google Map :-P However, I kept the class name the same ("s_google_map") to avoid confusion.

ged-odoo · 2025-04-01T13:32:01Z

addons/html_builder/static/src/plugins/google_maps_option/google_maps_api_key_dialog.js

@@ -0,0 +1,51 @@
+import { _t } from "@web/core/l10n/translation";


i'm not really reviewing this, but i have a quick remark: all this code should go into html_builder/static/src/website_builder. because it will be moved later in the website addon.

Ah ok! My whole google_maps_option folder?

yes, all that code should stay together

the html builder addon will become the generic foundation for website builder, mass mailing, report editor. so the goal is to have common code here, and specialized code in each corresponding addon

Understood, done. What about the tests though?

FrancoisGe · 2025-04-01T13:40:27Z

addons/website/static/src/js/editor/snippets.editor.js

@@ -273,8 +273,7 @@ export class WebsiteSnippetsMenu extends weSnippetEditor.SnippetsMenu {
                onMounted(() => this.props.onMounted(this.modalRef));
            }
            onClickSave() {
-                this.props.confirm(this.modalRef, this.state.apiKey);
-                this.props.close();
+                this.props.confirm(this.modalRef, this.state.apiKey, this.props.close);


why not
await this.props.confirm(...);
this.props.close()

This is just a cherry-pick of odoo#204139 so we don't have to wait for it to be merged to be able to use the snippet in the legacy website builder. It was mostly useful for me to test but I guess I could now remove the commit actually.

FrancoisGe · 2025-04-01T13:42:02Z

addons/html_builder/static/src/plugins/google_map_option/google_map_api_key_dialog.js

+ * @typedef {import('./google_map_option_plugin.js').ApiKeyValidation} ApiKeyValidation
+ */
+
+export const GoogleMapApiKeyDialog = class extends Component {


export class GoogleMapApiKeyDialog extends Component {

FrancoisGe · 2025-04-01T14:01:38Z

addons/html_builder/static/src/plugins/google_map_option/google_map_option.js

+            this.googleMapsAutocomplete = new google.maps.places.Autocomplete(
+                inputEl,
+                { types: [ 'geocode' ] },
+            );
+            google.maps.event.addListener(
+                this.googleMapsAutocomplete,
+                'place_changed',
+                this.onPlaceChanged.bind(this),
+            );


I m not sure it will work with the history. I don t know what is done by google.maps.places.Autocomplete

It's initializing the API, attaching it to the input element. This is in the sidebar option though, does the history have something to do with it?

FrancoisGe · 2025-04-01T14:03:58Z

addons/html_builder/static/src/plugins/google_map_option/google_map_option.scss

+            cursor: pointer;
+        }
+
+        /* Remove Google Map's own icon. */


It s maybe because we don t use the custom css. We try to use bootstrap if it s possible. The website builder sidebar style is not working correctly. A designer will probably try to make it better and use bootstrap

FrancoisGe · 2025-04-01T14:06:57Z

addons/html_builder/static/src/plugins/google_map_option/google_map_option_plugin.js

+                },
+            },
+            showDescription: {
+                apply: ({ editingElement }) => {


you need to define isApplied:() => editingElement.querySelector(".description")
clean: () => descriptionEl.remove()
apply () => append(...)

showDescription: { isApplied: ({ editingElement }) => !!editingElement.querySelector(".description"), apply: ({ editingElement }) => { editingElement.append(renderToElement("html_builder.GoogleMapDescription")); }, clean: ({ editingElement }) => { editingElement.querySelector(".description").remove(); }, },

Nice. Done.

FrancoisGe · 2025-04-01T14:10:42Z

addons/html_builder/static/src/plugins/google_map_option/google_map_service.js

+import { markup } from "@odoo/owl";
+import { escape } from "@web/core/utils/strings";
+
+registry.category("services").add("google_map", {


it s just a copy paste or you need to change something ?

Mostly a copy paste. Only change is I removed the dependency to "public.interactions":

odoo/addons/website/static/src/core/website_map_service.js

Lines 10 to 12 in ed7059b

dependencies: ["public.interactions", "notification"],

start(env, deps) {

const publicInteractions = deps["public.interactions"];

and its use at

odoo/addons/website/static/src/core/website_map_service.js

Lines 43 to 46 in ed7059b

for (const el of document.querySelectorAll("section.s_google_map")) {

publicInteractions.stopInteractions(el);

publicInteractions.startInteractions(el);

}

as we discussed last time.

FrancoisGe · 2025-04-01T14:13:10Z

addons/html_builder/static/src/plugins/google_map_option/google_map_option_plugin.js

+        this.mapsAPI = google.maps;
+        this.placesAPI = google.maps.places;
+        const place = await this.nearbySearch(editingElement, editingElement.dataset.mapGps);
+        if (place?.formatted_address) {
+            editingElement.dataset.pinAddress = place.formatted_address;
+        }
+        if (!place && !this.isGoogleMapsErrorBeingHandled) {
+            // Somehow the search failed but Google didn't trigger an error.
+            this.dependencies.remove.removeElement(editingElement);
+        }


It look strange to edit the website dom during the willStart of a Component. You will have issue with the history

You maybe want to add it when we add the google map snippet ?

You maybe want to add it when we add the google map snippet ?

That triggers a whole domino of weird issues*.

This is just removing the whole snippet in case of critical error though (it used to be this.trigger_up('user_value_widget_critical')). Is that so problematic?

* First one being that initializeAutocomplete is called before the google object is initialized. I can get around that by removing the on_add_element_handlers from BuilderOptionsPlugin as we had discussed so the option is not immediately started when dropping the snippet. But then if I await loadGoogleMaps in on_add_element_handlers it waits forever instead of opening the API key dialog so I guess something would have to be done in the snippet dropping plugin to prevent that. If I don't await it somehow the snippet has a <br> in it instead of the map - no idea why. My point is this little change triggers a whole mess of issues to deal with that seem to be to be quite a bit larger than the snippet, and more foundational.

Because it drives me crazy.

addons/html_builder/static/src/plugins/google_map_option/google_map_option_plugin.js

Goaman · 2025-04-01T15:10:48Z

addons/html_builder/static/src/plugins/google_map_option/google_map_option.js

+     * it sends a signal that the place changed.
+     */
+    onPlaceChanged() {
+        this.props.commitPlace(this.googleMapsAutocomplete.getPlace(), this.state.map);


Detail:
why is the prop have an imperative name? It means "GoogleMapOption" knows the parent implementation.
I would rather call the method this.props.onPlaceChanged to help with the mental model.
But it's up to you, feel free to resolve my comment and keep your code if you prefer.

Goaman · 2025-04-01T17:21:39Z

...r/static/src/website_builder/plugins/options/google_maps_option/google_maps_option_plugin.js

+     * @param {Element} editingElement
+     * @returns {Promise<void>}
+     */
+    async onGoogleMapsLoaded(editingElement) {


I suggest to change the method name that tell what the method is doing? updateElementMap?
Actually, I'm not sure what is the method supposed to do as I don't understand why we set pinAdress or remove the element. Reading the doc of the method did not help me understand what it is supposed to do or in which flow it should be called.

Why is it loadGoogleMaps calling this method?
It look like we want to update an element that need to load google map if I understand correctly.

async updateElementMap(editingElement, forceReconfigure) { const loaded = await this.loadGoogleMaps(editingElement, forceReconfigure) if (!loaded) return; ... }

Goaman · 2025-04-01T17:24:01Z

...r/static/src/website_builder/plugins/options/google_maps_option/google_maps_option_plugin.js

+        }
+        const place = await this.nearbySearch(editingElement, editingElement.dataset.mapGps);
+        if (place?.formatted_address) {
+            editingElement.dataset.pinAddress = place.formatted_address;


The mutation could be problematic with the history (same with mother other mutations in this PR)
Do we want a new step to be created?
If so, you should use the mutex (OperationPlugin.next)
If we don't want mutation and the attribute to be saved, add it in system_attributes.
Do we want it in the dom without a step? In that we need to think more about that case.

Zynton force-pushed the mysterious-egg-gmaps-age branch 16 times, most recently from c69d4b5 to fe40bfa Compare March 25, 2025 16:12

FrancoisGe force-pushed the master-mysterious-egg branch from f468b7f to 80b3d5d Compare March 27, 2025 12:21

ged-odoo force-pushed the master-mysterious-egg branch from 27a1402 to 8c6b756 Compare March 27, 2025 20:00

Zynton force-pushed the mysterious-egg-gmaps-age branch 11 times, most recently from 50652bf to fc4217f Compare April 1, 2025 07:12

Zynton force-pushed the mysterious-egg-gmaps-age branch from fc4217f to 1073222 Compare April 1, 2025 07:30

Zynton force-pushed the mysterious-egg-gmaps-age branch 3 times, most recently from ad1ffe4 to ed7059b Compare April 1, 2025 13:17

Zynton marked this pull request as ready for review April 1, 2025 13:17

Zynton changed the title ~~Google Maps~~ [ADD] html_builder: convert s_google_map Apr 1, 2025

Zynton commented Apr 1, 2025

View reviewed changes

ged-odoo reviewed Apr 1, 2025

View reviewed changes

FrancoisGe reviewed Apr 1, 2025

View reviewed changes

Zynton added 3 commits April 1, 2025 16:58

[ADD] html_builder: convert s_google_map

7844c89

[ADD] html_builder: add tests for Google Maps API key validation

3821637

[IMP] html_builder: rename Google Map to Google Maps everywhere

1c70092

Because it drives me crazy.

Zynton force-pushed the mysterious-egg-gmaps-age branch from ed7059b to 1c70092 Compare April 1, 2025 15:15

Goaman reviewed Apr 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADD] html_builder: convert s_google_map #4242

[ADD] html_builder: convert s_google_map #4242

Zynton commented Mar 24, 2025 •

edited

Loading

robodoo commented Mar 24, 2025

robodoo commented Apr 1, 2025

Zynton left a comment

Zynton Apr 1, 2025

Zynton Apr 1, 2025

Zynton Apr 1, 2025

Zynton Apr 1, 2025

FrancoisGe Apr 1, 2025

Zynton Apr 1, 2025

Zynton Apr 1, 2025

Zynton Apr 1, 2025

Zynton Apr 1, 2025

ged-odoo Apr 1, 2025

Zynton Apr 1, 2025

ged-odoo Apr 1, 2025

ged-odoo Apr 1, 2025

Zynton Apr 1, 2025

FrancoisGe Apr 1, 2025

Zynton Apr 1, 2025

FrancoisGe Apr 1, 2025

Zynton Apr 1, 2025

FrancoisGe Apr 1, 2025

Zynton Apr 1, 2025

FrancoisGe Apr 1, 2025

FrancoisGe Apr 1, 2025

Zynton Apr 1, 2025 •

edited

Loading

FrancoisGe Apr 1, 2025

Zynton Apr 1, 2025 •

edited

Loading

FrancoisGe Apr 1, 2025

FrancoisGe Apr 1, 2025

Zynton Apr 1, 2025 •

edited

Loading

Goaman Apr 1, 2025

Goaman Apr 1, 2025

Goaman Apr 1, 2025

		@@ -273,8 +273,7 @@ export class WebsiteSnippetsMenu extends weSnippetEditor.SnippetsMenu {
		onMounted(() => this.props.onMounted(this.modalRef));

		// @TODO mysterious-egg: should we still do this, which was just
		// done as a result of reporting a critical error?

		@@ -7,11 +7,11 @@ import { Component, useState, useRef } from "@odoo/owl";
		* @typedef {import('./google_map_option_plugin.js').ApiKeyValidation} ApiKeyValidation

		@@ -0,0 +1,51 @@
		import { _t } from "@web/core/l10n/translation";

	dependencies: ["public.interactions", "notification"],
	start(env, deps) {
	const publicInteractions = deps["public.interactions"];

	for (const el of document.querySelectorAll("section.s_google_map")) {
	publicInteractions.stopInteractions(el);
	publicInteractions.startInteractions(el);
	}

[ADD] html_builder: convert s_google_map #4242

Are you sure you want to change the base?

[ADD] html_builder: convert s_google_map #4242

Conversation

Zynton commented Mar 24, 2025 • edited Loading

robodoo commented Mar 24, 2025

robodoo commented Apr 1, 2025

Zynton 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

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

Choose a reason for hiding this comment

Zynton Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zynton Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zynton Apr 1, 2025 • 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

Zynton commented Mar 24, 2025 •

edited

Loading

Zynton Apr 1, 2025 •

edited

Loading

Zynton Apr 1, 2025 •

edited

Loading

Zynton Apr 1, 2025 •

edited

Loading