feat(*): Support for Bicycling Layer#1678
Conversation
- Added a single service to manage all map layers like transit and bicycling layers
Codecov Report
@@ Coverage Diff @@
## master #1678 +/- ##
==========================================
+ Coverage 39.7% 39.95% +0.24%
==========================================
Files 40 41 +1
Lines 1778 1802 +24
Branches 144 148 +4
==========================================
+ Hits 706 720 +14
- Misses 1071 1081 +10
Partials 1 1
Continue to review full report at Codecov.
|
| private _addedToManager: boolean = false; | ||
| private _id: string = (layerId++).toString(); | ||
| private static _transitLayerOptions: string[] = [ 'visible']; | ||
| private _name: MapLayerType = 'TransitLayer'; |
| visible: boolean; | ||
| } | ||
|
|
||
| export type MapLayerType = 'TransitLayer' | 'BicyclingLayer' | 'TrafficLayer'; |
There was a problem hiding this comment.
this is not a google map type, it doesn't belong here
| pixelOffset: Size; | ||
| } | ||
|
|
||
| export interface TransitLayer extends MVCObject { |
There was a problem hiding this comment.
Google maps has TransitLayer and BicycleLayer classes, but not MapLayer class.
There was a problem hiding this comment.
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
| * @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.
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.
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.
I agree to have one uniting manager. but I think there should be different methods in ApiWrapper and in google map types
| * @returns Promise<void> | ||
| */ | ||
| setOptions(layer: AgmTransitLayer|AgmBicyclingLayer, changes: SimpleChanges): Promise<void> { | ||
| const options = Object.keys(changes) |
There was a problem hiding this comment.
SimpleChanges is part of angular component infrastructure, and should not be leaked to our Manager classes. I recommend going back to how it was originally.
Additionally, keeping in mind that there is only ONE parameter for both bicycle layer, and transitlayer, it doesn't make sense do the entire Object.keys, filter, etc.
You can just do
if (changes['visible'] != null) {
return this.layerManager.setVisible(this, changes['visible'].currentValue);
}Of course keep the above lines of code in the directive's ngOnChanges method, and not in manager.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Hey, thanks for the changes. Three more small issues. In the meantime, I'll manually test the new code.
| transitLayer.setMap(options.visible ? map : null); | ||
| return transitLayer; | ||
| let newLayer: mapTypes.TransitLayer = new google.maps.TransitLayer(); | ||
| newLayer.setMap(options.visible ? map : null); |
ghost
merged commit 2 participants
https://developers.google.com/maps/documentation/javascript/trafficlayer#bicycling_layer