-
-
Notifications
You must be signed in to change notification settings - Fork 315
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.
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
[Map] Improve web performances by not calling "createInfoWindow" if unnecessary #2083
base: 2.x
Are you sure you want to change the base?
Conversation
marker.on('click', () => { | ||
this.createInfoWindow({ definition: infoWindow, marker, onMarkerClick: true }); | ||
}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
There was a problem hiding this comment.
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" ?
There was a problem hiding this comment.
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.
@@ -111,12 +111,14 @@ export default abstract class< | |||
protected createInfoWindow({ | |||
definition, | |||
marker, | |||
onMarkerClick = false, |
There was a problem hiding this comment.
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"
if (definition.opened || onMarkerClick) { | ||
infoWindow.open({ | ||
map: this.map, | ||
shouldFocus: false, | ||
anchor: marker, | ||
}); | ||
} |
There was a problem hiding this comment.
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
I feel like the current implementation could be improved, but i don't think we could/should avoid exposing this internal code.. and the loop seems a bit odd. Why not having separate create / open methods ? |
This afternoon we spoke a bit with @simondaigre, and he told me about InfoWindow performance issues when you have too many of them (and depending your device).
If we have 1.000 InfoWindow, hidden by default and only visible after clicking on a marker, why should be only create the InfoWindow at that time?
This PR introduce lazy-creation of InfoWindow, it should improve web-performances on the user's browsers.
I've used the following PHP code:
Benchmarks
I've did some benchmarks for both Google and Leaflet:
Google
connect()
takes 936ms instead of ~1,80s, we won ~900ms.Leaflet
connect()
takes ~1s instead of ~1,20s, we won ~200msThe next steps for performance optimizations are: