Skip to content
This repository was archived by the owner on Apr 7, 2020. It is now read-only.

Directive to search parent for leaflet directive #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

eugenehuangsg
Copy link
Contributor

I'm experimenting with separation of concerns using nested components within the leaflet directive. And I was thinking there is no real limitation on why we shouldn't allow ui-leaflet-draw to still work within the nested components. Is this okay?

Copy link
Contributor

@nmccready nmccready left a comment

Choose a reason for hiding this comment

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

Build dist and lets talk about this.

@@ -24,7 +24,7 @@ angular.module('ui-leaflet')
restrict: 'A'
scope: false
replace: false
require: ['leaflet']
require: ['^^leaflet']
Copy link
Contributor

Choose a reason for hiding this comment

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

https://docs.angularjs.org/guide/directive#creating-directives-that-communicate

The ^^ prefix means that this directive searches for the controller on its parents. (A ^ prefix would make the directive look for the controller on its own element or its parents; without any prefix, the directive would look on its own element only.)

So if this makes it work only by searching on it's parent then I am opposed (can be swayed). If it can work with both then I see no issue.

This is actually interesting because in reality I think this directive only depends on leaflet directive to get its relative map being loaded .

Anyway, if we move this to nesting I guess this would be a minor change or major? What do you think?

On breaking paradigm shift is making this nested as every attribute in ui-leaflet 1.X is on the same directive element.

Anyway, if travis didn't build the dist file this PR will need to do so. As I want to see if it still passes .

@eugenehuangsg eugenehuangsg force-pushed the master branch 2 times, most recently from f754404 to 63afade Compare March 30, 2017 15:54
@eugenehuangsg
Copy link
Contributor Author

eugenehuangsg commented Mar 30, 2017

@nmccready Yes, i absolutely agree that it should work for both parent and on its own element. (Should not break any existing paradigm). It was my oversight that read '^^' wrongly from the documentation. I have since changed it to only '^' and built dist using gulp. Thanks for pointing out the documentation clearly.

This is actually interesting because in reality I think this directive only depends on leaflet directive to get its relative map being loaded .

Yes, i think its very interesting too. So as long as this draw directive can find the leaflet's controller, everything should work as designed.

@eugenehuangsg
Copy link
Contributor Author

@nmccready Is this okay?

@nmccready
Copy link
Contributor

If we do this then we will need to bump the minor or major. Also from this main leaflets directive's standpoint all the directives share the same scope. So this is a break from the original design. Anyway, neither this or the other project is very active anymore. So I can be swayed either way.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants