-
Notifications
You must be signed in to change notification settings - Fork 817
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
feat(*): Support for Bicycling Layer #1678
Changes from 1 commit
eb28f9a
3c79eec
fe067dd
a21986b
8d5a804
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
import { Directive, Input, OnChanges, OnDestroy, OnInit, SimpleChanges } from '@angular/core'; | ||
import { MapLayerManager } from '../services/managers/map-layer-manager'; | ||
|
||
let layerId = 0; | ||
|
||
/* | ||
* This directive adds a bicycling layer to a google map instance | ||
* <agm-bicycling-layer [visible]="true|false"> <agm-bicycling-layer> | ||
* */ | ||
@Directive({ | ||
selector: 'agm-bicycling-layer' | ||
}) | ||
|
||
export class AgmBicyclingLayer implements OnInit, OnChanges, OnDestroy{ | ||
private _addedToManager: boolean = false; | ||
private _id: string = (layerId++).toString(); | ||
|
||
/** | ||
* Hide/show bicycling layer | ||
*/ | ||
@Input() visible: boolean = false; | ||
|
||
constructor( private _manager: MapLayerManager ) {} | ||
|
||
ngOnInit() { | ||
if (this._addedToManager) { | ||
return; | ||
} | ||
this._manager.addMapLayer(this, {visible: this.visible}); | ||
this._addedToManager = true; | ||
} | ||
|
||
ngOnChanges(changes: SimpleChanges) { | ||
if (!this._addedToManager) { | ||
return; | ||
} | ||
this._manager.setOptions(this, changes); | ||
} | ||
|
||
/** @internal */ | ||
id(): string { return this._id; } | ||
|
||
/** @internal */ | ||
name(): string { return 'BicyclingLayer'; } | ||
|
||
/** @internal */ | ||
toString(): string { return `AgmBicyclingLayer-${this._id.toString()}`; } | ||
|
||
/** @internal */ | ||
ngOnDestroy() { | ||
this._manager.deleteMapLayer(this); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,15 +105,16 @@ export class GoogleMapsAPIWrapper { | |
} | ||
|
||
/** | ||
* Creates a Google Map transit layer instance add it to map | ||
* @param {TransitLayerOptions} options - TransitLayerOptions options | ||
* @returns {Promise<TransitLayer>} a new transit layer object | ||
* Creates a MapLayer instance for a map | ||
* @param {MapLayerOptions} options - used for setting layer options | ||
* @param {string} name - the type of map layer to create | ||
* @returns {Promise<MapLayer>} a new map layer object | ||
*/ | ||
createTransitLayer(options: mapTypes.TransitLayerOptions): Promise<mapTypes.TransitLayer>{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really think that the google-maps-api-wrapper class should have separate methods for createTransitLayer, createBicycleLayer, createTrafficLayer since there is nothing in common between these, and it's bad practice to separate code execution by a passed in string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. I decided to go with one layerManager because Bicycling Layer and and Transit Layer has the exact methods. There is nothing different apart from the class names. I thought it would be an overkill to have 3 services with the same methods and properties, but I will separate them since that's how google does it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree to have one uniting manager. but I think there should be different methods in ApiWrapper and in google map types |
||
createMapLayer(options: mapTypes.MapLayerOptions, name: string): Promise<mapTypes.MapLayer>{ | ||
return this._map.then((map: mapTypes.GoogleMap) => { | ||
let transitLayer: mapTypes.TransitLayer = new google.maps.TransitLayer(); | ||
transitLayer.setMap(options.visible ? map : null); | ||
return transitLayer; | ||
let newLayer: mapTypes.MapLayer = new google.maps[`${name}`](); | ||
newLayer.setMap(options.visible ? map : null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extra tab before |
||
return newLayer; | ||
}); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -435,16 +435,18 @@ export interface KmlMouseEvent extends MouseEvent { | |
pixelOffset: Size; | ||
} | ||
|
||
export interface TransitLayer extends MVCObject { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Google maps has TransitLayer and BicycleLayer classes, but not MapLayer class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't you just split out the classes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. keeping one LayerManager, though, is a good idea, I just think that at the level of google apis, it should be separate since google keeps it separate |
||
export interface MapLayer extends MVCObject { | ||
getMap(): GoogleMap; | ||
setMap(map: GoogleMap): void; | ||
setOptions(options: TransitLayerOptions): void; | ||
setOptions(options: MapLayerOptions): void; | ||
} | ||
|
||
export interface TransitLayerOptions { | ||
export interface MapLayerOptions { | ||
visible: boolean; | ||
} | ||
|
||
export type MapLayerType = 'TransitLayer' | 'BicyclingLayer' | 'TrafficLayer'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not a google map type, it doesn't belong here |
||
|
||
export interface Data extends MVCObject { | ||
features: Feature[]; | ||
addGeoJson(geoJson: Object, options?: GeoJsonOptions): Feature[]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
import {NgZone} from '@angular/core'; | ||
import {TestBed, inject, fakeAsync} from '@angular/core/testing'; | ||
import {AgmTransitLayer} from '../../directives/transit-layer'; | ||
import {GoogleMapsAPIWrapper} from '../../services/google-maps-api-wrapper'; | ||
import {MapLayerManager} from './map-layer-manager'; | ||
import {AgmBicyclingLayer} from '../../directives/bicycling-layer'; | ||
|
||
describe('MapLayerManager', () => { | ||
beforeAll(() => { | ||
(<any>window).google = { | ||
maps: { | ||
MapLayer: class MapLayer { | ||
setMap = jest.fn(); | ||
getMap = jest.fn(); | ||
setOptions = jest.fn(); | ||
|
||
constructor() { | ||
|
||
} | ||
}, | ||
} | ||
}; | ||
}); | ||
|
||
beforeEach(() => { | ||
|
||
TestBed.configureTestingModule({ | ||
providers: [ | ||
{provide: NgZone, useFactory: () => new NgZone({enableLongStackTrace: true})}, | ||
{ | ||
provide: GoogleMapsAPIWrapper, | ||
useValue: { | ||
getNativeMap: () => Promise.resolve(), | ||
createMapLayer: jest.fn() | ||
} | ||
}, | ||
MapLayerManager, | ||
|
||
] | ||
}); | ||
}); // end beforeEach | ||
|
||
describe('Create a new transit layer', () => { | ||
|
||
it('should call mapsApiWrapper when creating a new transit layer', | ||
fakeAsync(inject( | ||
[MapLayerManager, GoogleMapsAPIWrapper], | ||
(mapLayerManager: MapLayerManager, apiWrapper: GoogleMapsAPIWrapper) => { | ||
const transitLayer = new AgmTransitLayer(mapLayerManager); | ||
const opt = {visible: false}; | ||
mapLayerManager.addMapLayer(transitLayer, opt); | ||
expect(apiWrapper.createMapLayer).toHaveBeenCalledWith(opt, 'TransitLayer'); | ||
|
||
}) | ||
) | ||
); | ||
}); | ||
|
||
describe('Create a new bicycling layer', () => { | ||
|
||
it('should call mapsApiWrapper when creating a new bicycling layer', | ||
fakeAsync(inject( | ||
[MapLayerManager, GoogleMapsAPIWrapper], | ||
(mapLayerManager: MapLayerManager, apiWrapper: GoogleMapsAPIWrapper) => { | ||
const bicyclingLayer = new AgmBicyclingLayer(mapLayerManager); | ||
const opt = {visible: true}; | ||
mapLayerManager.addMapLayer(bicyclingLayer, opt); | ||
expect(apiWrapper.createMapLayer).toHaveBeenCalledWith(opt, 'BicyclingLayer'); | ||
|
||
}) | ||
) | ||
); | ||
}); | ||
|
||
describe('Toggling visibility of a MapLayer', () => { | ||
|
||
it('should update the visibility of a map layer when the visibility option changes', fakeAsync( | ||
|
||
inject( | ||
[MapLayerManager, GoogleMapsAPIWrapper], | ||
(mapLayerManager: MapLayerManager, | ||
apiWrapper: GoogleMapsAPIWrapper) => { | ||
const newMapLayer = new AgmTransitLayer(mapLayerManager); | ||
newMapLayer.visible = true; | ||
|
||
const transitLayerInstance: any = { | ||
setMap: jest.fn(), | ||
getMap: jest.fn(), | ||
setOptions: jest.fn() | ||
|
||
}; | ||
|
||
(<jest.Mock>apiWrapper.createMapLayer).mockReturnValue( | ||
Promise.resolve(transitLayerInstance) | ||
); | ||
|
||
mapLayerManager.addMapLayer(newMapLayer, {visible: true}); | ||
expect(apiWrapper.createMapLayer).toHaveBeenCalledWith({visible: true}, 'TransitLayer'); | ||
|
||
newMapLayer.visible = false; | ||
mapLayerManager.toggleLayerVisibility(newMapLayer, {visible: false}).then(() => { | ||
expect(transitLayerInstance.setMap).toHaveBeenCalledWith(null); | ||
}); | ||
} | ||
) | ||
|
||
)); | ||
|
||
}); | ||
|
||
}); |
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.
Shouldn't this be static and final?