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

Adapt angular: ngeo.query #3290

Merged
merged 16 commits into from
Dec 20, 2017
Merged

Conversation

pfirpfel
Copy link
Contributor

No description provided.

@pfirpfel pfirpfel changed the title [wip] Adapt angular: ngeo.query Adapt angular: ngeo.query Dec 18, 2017
Copy link
Member

@ger-benjamin ger-benjamin left a comment

Choose a reason for hiding this comment

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

Thanks @pfirpfel
Can you validate / invalidate my comments @gberaudo ?

@@ -1,17 +1,23 @@
goog.provide('ngeo.Querent');
/**
* @module ngeo query namespace
Copy link
Member

Choose a reason for hiding this comment

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

what's that ? Do you know ?

Copy link
Member

Choose a reason for hiding this comment

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

We should not put @module annotations; they will be generated automatically later (in a specific format).

/**
* @module ngeo query namespace
*/
goog.provide('ngeo.query.bboxQueryDirective');
Copy link
Member

@ger-benjamin ger-benjamin Dec 19, 2017

Choose a reason for hiding this comment

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

Event if that's always an angular directive, can you names it "component ?".
That's a more generic term and In the future, we will change it to a component. So we (and our customers in their custom code) will have to update again the require.

(Also for other directives in this PR)

Copy link
Member

Choose a reason for hiding this comment

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

+1

@@ -3,7 +3,7 @@ goog.provide('gmf.QueryManager');
goog.require('ol.events');
goog.require('gmf');
goog.require('gmf.Themes');
goog.require('ngeo.Query');
goog.require('ngeo.query.Query');
Copy link
Member

Choose a reason for hiding this comment

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

ngeo.query.Service ? query Query looks weird (In my own opinion)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, should be Service.


ngeo.module.requires.push(ngeo.query.mapQueryDirective.module.name);

ngeo.query.mapQueryDirective.module.directive('ngeoMapQuery', ngeo.query.mapQueryDirective);
Copy link
Member

Choose a reason for hiding this comment

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

I think we said to export the Angular module for directives and components (not the directive function).

ngeo.query.mapQueryDirective = angular.module('ngeoMapQuery', [
  ngeo.query.MapQuerent.name,
]);

Copy link
Member

Choose a reason for hiding this comment

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

+1
This file is strange. That should be:
ngeo.query.mapQueryDirective = angular.module...
ngeo.module.requires....
...
ngeo.query.mapQueryDirective.directive_ = function...
...
ngeo.query.mapQueryDirective.directive('ngeoMapQuery', ngeo.query.mapQueryDirective.directive_);

no ?

@pfirpfel pfirpfel merged commit 0815d4f into camptocamp:master Dec 20, 2017
@pfirpfel pfirpfel deleted the adapt-angular-ngeo-query branch December 20, 2017 09:25
@pfirpfel pfirpfel removed the Ongoing label Dec 20, 2017
@sbrunner sbrunner added this to the 2.3 milestone Apr 12, 2018
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.

4 participants