Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Map] Improve web performances by not calling "createInfoWindow" if unnecessary #2083

Draft
wants to merge 1 commit into
base: 2.x
Choose a base branch
from
Draft

[Map] Improve web performances by not calling "createInfoWindow" if unnecessary #2083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/Map/assets/dist/abstract_map_controller.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,15 @@ export default abstract class<MapOptions, Map, MarkerOptions, Marker, InfoWindow
}): Map;
createMarker(definition: MarkerDefinition<MarkerOptions, InfoWindowOptions>): Marker;
protected abstract doCreateMarker(definition: MarkerDefinition<MarkerOptions, InfoWindowOptions>): Marker;
protected createInfoWindow({ definition, marker, }: {
protected createInfoWindow({ definition, marker, onMarkerClick, }: {
definition: MarkerDefinition<MarkerOptions, InfoWindowOptions>['infoWindow'];
marker: Marker;
onMarkerClick?: boolean;
}): InfoWindow;
protected abstract doCreateInfoWindow({ definition, marker, }: {
protected abstract doCreateInfoWindow({ definition, marker, onMarkerClick, }: {
definition: MarkerDefinition<MarkerOptions, InfoWindowOptions>['infoWindow'];
marker: Marker;
onMarkerClick: boolean;
}): InfoWindow;
protected abstract doFitBoundsToMarkers(): void;
protected abstract dispatchEvent(name: string, payload: Record<string, unknown>): void;
Expand Down
4 changes: 2 additions & 2 deletions src/Map/assets/dist/abstract_map_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ class default_1 extends Controller {
this.markers.push(marker);
return marker;
}
createInfoWindow({ definition, marker, }) {
createInfoWindow({ definition, marker, onMarkerClick = false, }) {
this.dispatchEvent('info-window:before-create', { definition, marker });
const infoWindow = this.doCreateInfoWindow({ definition, marker });
const infoWindow = this.doCreateInfoWindow({ definition, marker, onMarkerClick });
this.dispatchEvent('info-window:after-create', { infoWindow, marker });
this.infoWindows.push(infoWindow);
return infoWindow;
Expand Down
6 changes: 5 additions & 1 deletion src/Map/assets/src/abstract_map_controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,14 @@ export default abstract class<
protected createInfoWindow({
definition,
marker,
onMarkerClick = false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this should be exposed like this, seems to complexify the contract, while not beeing this "explanatory"

}: {
definition: MarkerDefinition<MarkerOptions, InfoWindowOptions>['infoWindow'];
marker: Marker;
onMarkerClick?: boolean;
}): InfoWindow {
this.dispatchEvent('info-window:before-create', { definition, marker });
const infoWindow = this.doCreateInfoWindow({ definition, marker });
const infoWindow = this.doCreateInfoWindow({ definition, marker, onMarkerClick });
this.dispatchEvent('info-window:after-create', { infoWindow, marker });

this.infoWindows.push(infoWindow);
Expand All @@ -127,9 +129,11 @@ export default abstract class<
protected abstract doCreateInfoWindow({
definition,
marker,
onMarkerClick,
}: {
definition: MarkerDefinition<MarkerOptions, InfoWindowOptions>['infoWindow'];
marker: Marker;
onMarkerClick: boolean;
}): InfoWindow;

protected abstract doFitBoundsToMarkers(): void;
Expand Down
3 changes: 2 additions & 1 deletion src/Map/src/Bridge/Google/assets/dist/map_controller.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ export default class extends AbstractMapController<MapOptions, google.maps.Map,
options: MapOptions;
}): google.maps.Map;
protected doCreateMarker(definition: MarkerDefinition<google.maps.marker.AdvancedMarkerElementOptions, google.maps.InfoWindowOptions>): google.maps.marker.AdvancedMarkerElement;
protected doCreateInfoWindow({ definition, marker, }: {
protected doCreateInfoWindow({ definition, marker, onMarkerClick, }: {
definition: MarkerDefinition<google.maps.marker.AdvancedMarkerElementOptions, google.maps.InfoWindowOptions>['infoWindow'];
marker: google.maps.marker.AdvancedMarkerElement;
onMarkerClick: boolean;
}): google.maps.InfoWindow;
private createTextOrElement;
private closeInfoWindowsExcept;
Expand Down
21 changes: 12 additions & 9 deletions src/Map/src/Bridge/Google/assets/dist/map_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,34 +52,37 @@ class default_1 extends AbstractMapController {
map: this.map,
});
if (infoWindow) {
this.createInfoWindow({ definition: infoWindow, marker });
if (infoWindow.opened) {
this.createInfoWindow({ definition: infoWindow, marker });
}
else {
marker.addListener('click', () => {
this.createInfoWindow({ definition: infoWindow, marker, onMarkerClick: true });
});
}
}
return marker;
}
doCreateInfoWindow({ definition, marker, }) {
doCreateInfoWindow({ definition, marker, onMarkerClick, }) {
const { headerContent, content, extra, rawOptions = {}, ...otherOptions } = definition;
const infoWindow = new _google.maps.InfoWindow({
headerContent: this.createTextOrElement(headerContent),
content: this.createTextOrElement(content),
...otherOptions,
...rawOptions,
});
if (definition.opened) {
if (definition.opened || onMarkerClick) {
infoWindow.open({
map: this.map,
shouldFocus: false,
anchor: marker,
});
}
marker.addListener('click', () => {
if (onMarkerClick) {
if (definition.autoClose) {
this.closeInfoWindowsExcept(infoWindow);
}
infoWindow.open({
map: this.map,
anchor: marker,
});
});
}
return infoWindow;
}
createTextOrElement(content) {
Expand Down
21 changes: 12 additions & 9 deletions src/Map/src/Bridge/Google/assets/src/map_controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,13 @@ export default class extends AbstractMapController<
});

if (infoWindow) {
this.createInfoWindow({ definition: infoWindow, marker });
if (infoWindow.opened) {
this.createInfoWindow({ definition: infoWindow, marker });
} else {
marker.addListener('click', () => {
this.createInfoWindow({ definition: infoWindow, marker, onMarkerClick: true });
});
}
}

return marker;
Expand All @@ -130,12 +136,14 @@ export default class extends AbstractMapController<
protected doCreateInfoWindow({
definition,
marker,
onMarkerClick,
}: {
definition: MarkerDefinition<
google.maps.marker.AdvancedMarkerElementOptions,
google.maps.InfoWindowOptions
>['infoWindow'];
marker: google.maps.marker.AdvancedMarkerElement;
onMarkerClick: boolean;
}): google.maps.InfoWindow {
const { headerContent, content, extra, rawOptions = {}, ...otherOptions } = definition;

Expand All @@ -146,24 +154,19 @@ export default class extends AbstractMapController<
...rawOptions,
});

if (definition.opened) {
if (definition.opened || onMarkerClick) {
infoWindow.open({
map: this.map,
shouldFocus: false,
anchor: marker,
});
}
Comment on lines +157 to 163
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confusing to me... this mixes runtime values and definition values


marker.addListener('click', () => {
if (onMarkerClick) {
if (definition.autoClose) {
this.closeInfoWindowsExcept(infoWindow);
}

infoWindow.open({
map: this.map,
anchor: marker,
});
});
}

return infoWindow;
}
Expand Down
3 changes: 2 additions & 1 deletion src/Map/src/Bridge/Leaflet/assets/dist/map_controller.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ export default class extends AbstractMapController<MapOptions, typeof L.Map, Mar
options: MapOptions;
}): L.Map;
protected doCreateMarker(definition: MarkerDefinition): L.Marker;
protected doCreateInfoWindow({ definition, marker, }: {
protected doCreateInfoWindow({ definition, marker, onMarkerClick, }: {
definition: MarkerDefinition['infoWindow'];
marker: L.Marker;
onMarkerClick: boolean;
}): L.Popup;
protected doFitBoundsToMarkers(): void;
}
Expand Down
13 changes: 10 additions & 3 deletions src/Map/src/Bridge/Leaflet/assets/dist/map_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,21 @@ class map_controller extends AbstractMapController {
const { position, title, infoWindow, extra, rawOptions = {}, ...otherOptions } = definition;
const marker = L.marker(position, { title, ...otherOptions, ...rawOptions }).addTo(this.map);
if (infoWindow) {
this.createInfoWindow({ definition: infoWindow, marker });
if (infoWindow.opened) {
this.createInfoWindow({ definition: infoWindow, marker });
}
else {
marker.on('click', () => {
this.createInfoWindow({ definition: infoWindow, marker, onMarkerClick: true });
});
Comment on lines +45 to +47
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not seem a good idea to run every time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"createInfoWindow" will be called on every "click" ?

Copy link
Member Author

@Kocal Kocal Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes you're right.

Hum, I may have better ideas to rewrite the PR, but I don't have time right now, let's put it in draft.

}
}
return marker;
}
doCreateInfoWindow({ definition, marker, }) {
doCreateInfoWindow({ definition, marker, onMarkerClick, }) {
const { headerContent, content, extra, rawOptions = {}, ...otherOptions } = definition;
marker.bindPopup([headerContent, content].filter((x) => x).join('<br>'), { ...otherOptions, ...rawOptions });
if (definition.opened) {
if (definition.opened || onMarkerClick) {
marker.openPopup();
}
const popup = marker.getPopup();
Expand Down
12 changes: 10 additions & 2 deletions src/Map/src/Bridge/Leaflet/assets/src/map_controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,13 @@ export default class extends AbstractMapController<
const marker = L.marker(position, { title, ...otherOptions, ...rawOptions }).addTo(this.map);

if (infoWindow) {
this.createInfoWindow({ definition: infoWindow, marker });
if (infoWindow.opened) {
this.createInfoWindow({ definition: infoWindow, marker });
} else {
marker.on('click', () => {
this.createInfoWindow({ definition: infoWindow, marker, onMarkerClick: true });
});
}
}

return marker;
Expand All @@ -72,14 +78,16 @@ export default class extends AbstractMapController<
protected doCreateInfoWindow({
definition,
marker,
onMarkerClick,
}: {
definition: MarkerDefinition['infoWindow'];
marker: L.Marker;
onMarkerClick: boolean;
}): L.Popup {
const { headerContent, content, extra, rawOptions = {}, ...otherOptions } = definition;

marker.bindPopup([headerContent, content].filter((x) => x).join('<br>'), { ...otherOptions, ...rawOptions });
if (definition.opened) {
if (definition.opened || onMarkerClick) {
marker.openPopup();
}

Expand Down