Skip to content

Comments

map event listeners enabled#16

Open
pavvell wants to merge 4 commits intojust-boris:masterfrom
pavvell:master
Open

map event listeners enabled#16
pavvell wants to merge 4 commits intojust-boris:masterfrom
pavvell:master

Conversation

@pavvell
Copy link

@pavvell pavvell commented Mar 4, 2016

Yandex maps service emits events that you may want to listen in you controllers/directives (like when user clicks on a balloon for example). This functionality is enabled now, you can listen any event you want by adding a list of them in new "events" attribute. Event Docs.

@just-boris
Copy link
Owner

Thank you for a pull-request! The idea looks good, but it needs better implementation.

First of all, how are you going to detect map instance if you have more than one map on some page?

I think it would be better to do it by attributes:

<yandex-map ymap-balloonopen="doSomething($event)"></yandex-map>

It will be very similar how input directive in core angular works.

<input type="text" ng=model="myModel" ng-change="doSomething($event)">

You can see how it is implemented in angular sources and do it here in the similar fashion

@pavvell
Copy link
Author

pavvell commented Mar 4, 2016

Good point! Working on it, will publish soon.

@pavvell
Copy link
Author

pavvell commented Mar 11, 2016

Hey, Boris! I've updated the event listeners mechanism, now it works as you recommended:

<yandex-map ymap-balloonopen="doSomething($event)"></yandex-map>

To achieve this functionalty i've added new constant "EVENTS" and configurable parameter "eventPrefixInDirective" in ymapsConfig constant.

I've added description in readme as well.

@just-boris
Copy link
Owner

Thank you, good job!

But registerEventEmitters looks like a hack. You can do everything just in the controller, if you need attributes, you can inject them in the controller via $attrs injectable.

Also, you should write few unit tests that ymap-balloonopen works, but something like ng-balloonopen doesn't.

And feel free to ask me some questions if you need.

@pavvell
Copy link
Author

pavvell commented Mar 12, 2016

Got it! Thanks for the reasonable feedback. Will update soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants