Skip to content

feat(google-maps): allow for info window focus behavior to be customized #23831

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

Merged
merged 1 commit into from
Jan 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 30 additions & 1 deletion src/google-maps/map-info-window/map-info-window.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,13 @@ describe('MapInfoWindow', () => {
expect(infoWindowSpy.close).toHaveBeenCalled();

infoWindowComponent.open(fakeMarkerComponent);
expect(infoWindowSpy.open).toHaveBeenCalledWith(mapSpy, fakeMarker);
expect(infoWindowSpy.open).toHaveBeenCalledWith(
jasmine.objectContaining({
map: mapSpy,
anchor: fakeMarker,
shouldFocus: undefined,
}),
);
});

it('should not try to reopen info window multiple times for the same marker', () => {
Expand Down Expand Up @@ -224,6 +230,29 @@ describe('MapInfoWindow', () => {
infoWindowComponent.open();
expect(infoWindowSpy.open).toHaveBeenCalledTimes(1);
});

it('should allow for the focus behavior to be changed when opening the info window', () => {
const fakeMarker = {} as unknown as google.maps.Marker;
const fakeMarkerComponent = {
marker: fakeMarker,
getAnchor: () => fakeMarker,
} as unknown as MapMarker;
const infoWindowSpy = createInfoWindowSpy({});
createInfoWindowConstructorSpy(infoWindowSpy).and.callThrough();

const fixture = TestBed.createComponent(TestApp);
const infoWindowComponent = fixture.debugElement
.query(By.directive(MapInfoWindow))!
.injector.get<MapInfoWindow>(MapInfoWindow);
fixture.detectChanges();

infoWindowComponent.open(fakeMarkerComponent, false);
expect(infoWindowSpy.open).toHaveBeenCalledWith(
jasmine.objectContaining({
shouldFocus: false,
}),
);
});
});

@Component({
Expand Down
10 changes: 8 additions & 2 deletions src/google-maps/map-info-window/map-info-window.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ export class MapInfoWindow implements OnInit, OnDestroy {
* Opens the MapInfoWindow using the provided anchor. If the anchor is not set,
* then the position property of the options input is used instead.
*/
open(anchor?: MapAnchorPoint) {
open(anchor?: MapAnchorPoint, shouldFocus?: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion: I'm wondering if we should pass these things through an object literal, making the API more readable when more options are added? I find it generally more clear like open({shouldFocus: false}), than open(undefined, false)

Copy link
Member Author

Choose a reason for hiding this comment

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

Changing it to an object literal would be a breaking change at this point. We could support both syntaxes for a while, but I don't think it's a big deal considering that there are only two parameters.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW I also would prefer an object here (with backwards compat), but don't feel strongly enough to block the change

https://martinfowler.com/bliki/FlagArgument.html

this._assertInitialized();
const anchorObject = anchor ? anchor.getAnchor() : undefined;

Expand All @@ -178,7 +178,13 @@ export class MapInfoWindow implements OnInit, OnDestroy {
// case where the window doesn't have an anchor, but is placed at a particular position.
if (this.infoWindow.get('anchor') !== anchorObject || !anchorObject) {
this._elementRef.nativeElement.style.display = '';
this.infoWindow.open(this._googleMap.googleMap, anchorObject);

// The config is cast to `any`, because the internal typings are out of date.
this.infoWindow.open({
map: this._googleMap.googleMap,
anchor: anchorObject,
shouldFocus,
} as any);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/google-maps/testing/fake-google-map-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ export function createInfoWindowSpy(
'get',
]);
infoWindowSpy.addListener.and.returnValue({remove: () => {}});
infoWindowSpy.open.and.callFake((_map: any, target: any) => (anchor = target));
infoWindowSpy.open.and.callFake((config: any) => (anchor = config.anchor));
infoWindowSpy.close.and.callFake(() => (anchor = null));
infoWindowSpy.get.and.callFake((key: string) => (key === 'anchor' ? anchor : null));
return infoWindowSpy;
Expand Down
2 changes: 1 addition & 1 deletion tools/public_api_guard/google-maps/google-maps.md
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ export class MapInfoWindow implements OnInit, OnDestroy {
ngOnDestroy(): void;
// (undocumented)
ngOnInit(): void;
open(anchor?: MapAnchorPoint): void;
open(anchor?: MapAnchorPoint, shouldFocus?: boolean): void;
// (undocumented)
set options(options: google.maps.InfoWindowOptions);
// (undocumented)
Expand Down