refactor(google-maps): simplify marker internal setup #20975
Conversation
crisbeto
added
P3
google-cla
bot
added
the
cla: yes
| * Options used to configure the marker. | ||
| * See: developers.google.com/maps/documentation/javascript/reference/marker#MarkerOptions | ||
| */ | ||
| @Input() options: google.maps.MarkerOptions; |
There was a problem hiding this comment.
| @Input() options: google.maps.MarkerOptions; | |
| @Input() options: Readonly<google.maps.MarkerOptions>; |
We could enforce that the whole object needs to be set
There was a problem hiding this comment.
Makes sense, this might break some users who are using this incorrectly (or just not carefully) though.
crisbeto
force-pushed
the
map-marker-rework
branch
from
c4e4075 to
f92990e
Compare
|
The setters have been re-added. @mbehrlich, can you take another look? |
| this._assertInitialized(); | ||
| this.marker.setTitle(title); | ||
| if (marker) { | ||
| marker.setOptions(this._combineOptions()); |
There was a problem hiding this comment.
I think we need to use the "changes" object in ngOnChanges to prevent these methods from running if their particular input has not changed. The reason is that we don't necessarily want to re-set all of the options when we're changing one particular input. Imagine a situation where the developer sets an initial set of options for a marker. Then the user drags the marker across the map. Then they trigger a change to the title. This way of doing it will set all of the initial inputs that have not been updated yet, including the initial position, causing the marker to return to its original position.
If we use the changes object to see what input is actually being changed, we can just run that setter method and avoid this problem (similar to how I originally watched for various changes instead of running setOptions for every change).
There was a problem hiding this comment.
I was thinking about that, but my assumption was that the Google Maps API would know not to update the map if the values haven't changed.
There was a problem hiding this comment.
I believe that's correct, but in the situation I outlined, it would appear that the values had changed to the internal Marker class. Basically, if we initialize a marker with these options:
[position]="{lat: 23, lng: 10}"
[title]="'new marker'"
[draggable]="true"
Since draggable is true, users can drag the marker anywhere they want. This will update the position on the internal marker, but it won't mutate the object that the develop input. So, let's say they drag it to {lat: 16, lng: -2}.
Then, let's say a user clicks on something that triggers an update to the title. This code will call setOptions with
{
position: {lat: 23, lng: 10},
title: 'updated title',
draggable: true,
}
This snaps the marker out of the spot the user dragged it to, even though we would it expect to only update the title.
By relying on the "changes" object, we can check what input is actually being changed and then just run the appropriate setter, in this case "setTitle" which will update the title and not change the position.
There was a problem hiding this comment.
That makes sense. Changing...
Currently the various Google Maps sub-components are set up so that each input has a setter, a `BehaviorSubject` and a method to watch it for changes. This doesn't scale very well on components with lots of inputs so these changes use an alternate one where we synchronously update the marker state whenever an input changes.
crisbeto
force-pushed
the
map-marker-rework
branch
from
f92990e to
f2366b1
Compare
Currently the various Google Maps sub-components are set up so that each input has a setter, a `BehaviorSubject` and a method to watch it for changes. This doesn't scale very well on components with lots of inputs so these changes use an alternate one where we synchronously update the marker state whenever an input changes. (cherry picked from commit 0e88125)
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently the various Google Maps sub-components are set up so that each input has a setter, a
BehaviorSubjectand a method to watch it for changes. This doesn't scale very well on components with lots of inputs so these changes use an alternate one where we synchronously update the marker state whenever an input changes.Note: This PR is meant to be a proof-of-concept so that we can discuss the approach. If we agree that it's the way forward, it can be applied to the other map components.