Skip to content
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

Add ngeo popover component #927

Merged
merged 1 commit into from Apr 18, 2016

Conversation

olivierSemet
Copy link
Contributor

PR for the new ngeo component popover. The first goal of this component is to display actions capabilities for groups of layers or individual layers into the layertree but there is no limitation for other use at the moment.

Behavior of the component:

  • Clicking on the anchor tag SHOULD toogle the popover
  • If multiple anchors exist, only one popover can be displayed at a time
  • Each anchor is linked to its own popover, meaning it exists as much popovers as anchors
  • Clicking into the popover SHOULD NOT dismiss it
  • Clicking somewhere else SHOULD dismiss the popover

Check the example on my GH pages here

ngeo.popoverDirective = function() {
return {
restrict: 'A',
scope: {},
Copy link
Member

Choose a reason for hiding this comment

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

scope should not be isolate, otherwise we can't manage inner content of the popover from upper controllers.

@fgravin
Copy link
Member

fgravin commented Mar 22, 2016

Please try in your example to generate inner content of your popover from mainController model.
Fix scope inheritance from issues you get.

scope: {},
link: function(scope, elem, attrs, controllers, transclude) {
var ngeoPopoverCtrl = controllers[0];
ngeoPopoverCtrl.bodyElm = transclude();
Copy link
Member

Choose a reason for hiding this comment

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

Can you try without transclude and just set ngeoPopoverCtrl.bodyElm from element first child.
Translude is actually used for something specfic that is not your usecase.
I wonder if the transclude function doesn't ensure the content is compiled though.
To test.

@fgravin
Copy link
Member

fgravin commented Mar 22, 2016

The layertree will change a lot so the popover will be generated and destroyed, be sure you clean everything well when they are destroyed.

@fgravin
Copy link
Member

fgravin commented Mar 22, 2016

Good work, I'v made some comments though.

In the end, i think that one directive should have been enough for the whole thing, your approach is good but might introduce useless complexity.

@olivierSemet olivierSemet force-pushed the adding-popover-component branch 3 times, most recently from a3fb6e5 to 790a748 Compare March 22, 2016 13:47
@olivierSemet
Copy link
Contributor Author

@fgravin following your advices, I pushed a new version.

@fgravin
Copy link
Member

fgravin commented Mar 22, 2016

Looks good thanks @olivierSemet.
I'll wait the PSC validates the spec to merge it.

content: ngeoPopoverCtrl.bodyElm
});

scope.$on('$destroy', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Need to check it's needed (wait to integrate it in desktop app for that).
The element will be removed anyway, i hope jquery unregister all events when removing an element.

link : function(scope, elem, attrs, controller) {
var ngeoPopoverCtrl = controller;

ngeoPopoverCtrl.anchorElm .on('hidden.bs.popover', function() {

Choose a reason for hiding this comment

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

minor: space in front of .on

@olivierSemet olivierSemet force-pushed the adding-popover-component branch 6 times, most recently from 3b46a82 to afb387c Compare March 24, 2016 09:04
@sbrunner sbrunner added this to the 2.1 milestone Mar 24, 2016
@olivierSemet olivierSemet force-pushed the adding-popover-component branch 4 times, most recently from 8b159cc to 9662e0b Compare April 18, 2016 08:26
@fgravin fgravin changed the title [wip] add ngeo popover component Add ngeo popover component Apr 18, 2016
@fgravin
Copy link
Member

fgravin commented Apr 18, 2016

Good work.
We know that there is a potential limitation when require is not correctly handled cause of the transclude linked function usage.
But merging cause we didn't find good solution for now.

@fgravin fgravin merged commit 719cf31 into camptocamp:master Apr 18, 2016
@olivierSemet olivierSemet deleted the adding-popover-component branch April 20, 2016 11:13
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.

None yet

4 participants