search

AckerApple / agm-overlays

Custom marker overlay for the @agm/core package
MIT License
25 stars 24 forks source link

replace overlayImage by overlayLayer #23

Closed guillaumerems closed 4 years ago

guillaumerems commented 5 years ago

Hi,

I did this update to be able to put a marker over an image. If you could check and merge it

Thanks ;)

AckerApple commented 5 years ago

Could you give me more details. Why remove overlayImage? And what is overlayLayer.

Please elaborate as I do not have enough context to know what’s going on with your pull request

guillaumerems commented 5 years ago

I do want to display a marker over an image.

With overlayImage, the marker is behind the image so we do not see it. With overlayLayer, the marker is over the image so we can see the marker, and the image below.

AckerApple commented 5 years ago

I think I want to keep “overlayImage” and mixin “overlayLayer”.

I forgot what overlayImage does and believe that because it’s just inherited for agm-core marker directive.

Looks like I’m gonna have to look those both up before I blindly accept you code that deletes something.

I’ll be in touch within 6 days I believe

AckerApple commented 5 years ago

Sorry, I checked out your code and it fails.

Issue

I checked out your repo, I pulled the branch overlayLayer, and ran the following:

$ npm install
$ npm run watch

I also don't like this code. I think you should only use the agm-overlay directive for html content. It sounds like you are trying to do both an image and html in just one agm-overlay. Maybe you should use a separate marker?

I think agm-core's provided MarkerManager directive does all that it does using overlayImage. This library, agm-overlays, sorta fakes being a marker so that it can work with MarkerManager provider of agm-core.

To close, I have another pull request from someone else I'm moving onto. I think you might be in for some huge changes if you want to use overlayLayer.

Good luck. Rejected, for now

AckerApple commented 5 years ago

I accepted the other pull request. Be sure to pull before you work on this further.

I think since you are using overlayLayer, the click action is not being attached.

guillaumerems commented 5 years ago

No problem I will keep it on my branch as I must be able to get a marker over an image.