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

Modularize recenter #3204

Merged
merged 1 commit into from
Dec 6, 2017
Merged

Modularize recenter #3204

merged 1 commit into from
Dec 6, 2017

Conversation

ger-benjamin
Copy link
Member

*/
ngeo.recenter.directive = angular.module('ngeoRecenter', []);

const ngeoModule = ngeo.module;
Copy link
Member Author

Choose a reason for hiding this comment

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

I understand why I have to create this const, but not why it was not required in the gmf.search.component...

Copy link
Member

Choose a reason for hiding this comment

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

@ger-benjamin, no it is not correct to use this const here: you can not create a local constant, in a goog.provide file.
Do you have an error when using ngeo.module directly?

goog.require('ol.Map');
goog.require('ol.View');
goog.require('ol.layer.Tile');
goog.require('ol.source.OSM');

goog.require('ngeo.recenter.module');
Copy link
Member

Choose a reason for hiding this comment

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

According to https://jira.camptocamp.com/browse/GSGMF-172 the recenter directive is part of the map top level module. So in theory I would expect the map module to be required here.
@sbrunner, what do you think about it? Should examples require the top level modules or directly the directives they use?

Copy link
Member

Choose a reason for hiding this comment

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

In the applications we will use in priority the tom level module, I think that's should be the same in the examples :-)

@@ -1,7 +1,15 @@
goog.provide('ngeo.recenterDirective');
goog.provide('ngeo.recenter.directive');
Copy link
Member

Choose a reason for hiding this comment

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

Should be in src/map/recenter.js. @sbrunner, do you want to add a note in https://jira.camptocamp.com/browse/GSGMF-172 explaining the files need to be renamed (not moved in a directive subdirectory)?

Then the symbol should be ngeo.map.recenter. (I would drop the directive suffix).

Copy link
Member

Choose a reason for hiding this comment

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

+1

*/
ngeo.recenter.directive = angular.module('ngeoRecenter', []);

const ngeoModule = ngeo.module;
Copy link
Member

Choose a reason for hiding this comment

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

@ger-benjamin, no it is not correct to use this const here: you can not create a local constant, in a goog.provide file.
Do you have an error when using ngeo.module directly?

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

Choose a reason for hiding this comment

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

This module is not necessary (there is no recenter top level module). You can recycle it by renaming it ngeo.map.module and moving it there if it does not already exist.

@ger-benjamin
Copy link
Member Author

Thanks @sbrunner @gberaudo . I'll wait to merge #3206 then address your comment

@@ -1,7 +1,15 @@
goog.provide('ngeo.recenterDirective');
goog.provide('ngeo.recenter.directive');
Copy link
Member

Choose a reason for hiding this comment

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

+1

@ger-benjamin ger-benjamin merged commit 6f0f896 into master Dec 6, 2017
@ger-benjamin ger-benjamin deleted the modularize_recenter branch December 6, 2017 07:02
@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.

3 participants