MAP-16: Enhance the macro to support the display of maps based on OpenStreetMap #9

slauriere · 2018-06-27T13:40:29Z

Create two distinct JSX: LeafletMap and GoogleMap
Add a parameter named "library" for choosing the JavaScript library to be used
Add a parameter named "tiles" for choosing the tile provider
Simplify some translation messages

…nStreetMap

slauriere · 2018-06-27T13:44:03Z

Hi @vmassol, Since Alex is off until next week, can I ask you to review this pull request and to merge it into the master branch if it sounds ok to you?

- open OSM directions on click - make it possible to use % or px for width/height definition

acotiuga · 2018-07-06T11:21:34Z

src/main/resources/MapMacro/Code/Configuration.xml

    </class>
    <property>
-      <apiKey/>
+      <apiKey>AIzaSyBuiJnKVxb9XPoSF8Rv8XC6sZBMzidQPF8</apiKey>


I think we shouldn't commit any key here, since it has to be linked to a google account. Is this your key?

acotiuga · 2018-07-06T11:27:37Z

src/main/resources/MapMacro/Code/LeafletMap.xml

+    initialize: function(element, options) {
+      this.element = $(element);
+      // Default option values
+      this.options = (options !== undefined &amp;&amp; typeof options == 'object' ) ? options : {};


The alignment rule you applied doesn't seem to be consistent. Why isn't this line following the rule used in the next ones?

Thank you Alex, indeed, I had overlooked the alignment. It is meant to be fixed it in the latest commit 5233227

acotiuga · 2018-07-06T11:28:11Z

src/main/resources/MapMacro/Code/LeafletMap.xml

+      // Default option values
+      this.options = (options !== undefined &amp;&amp; typeof options == 'object' ) ? options : {};
+      this.options.zoom         = this.options.zoom !== undefined         ? this.options.zoom : 14;
+      this.options.latLng       = this.options.latLng !== undefined       ? this.options.latLng.split(','): [48.864716, 2.349014];


I would have get rid of these extra whitespaces and keep the lines in max 120 chars.

acotiuga · 2018-07-06T12:15:30Z

src/main/resources/XWiki/MapMacroTranslations.fa.xml

-  <contentUpdateDate>1525702967000</contentUpdateDate>
+  <date>1530017451000</date>
+  <contentUpdateDate>1530017451000</contentUpdateDate>
  <version>1.1</version>


These kind of changes should be avoided in commits.

acotiuga · 2018-07-06T12:26:47Z

src/main/resources/XWiki/MapMacroTranslations.xml

-rendering.macro.map.apiKey.invalid=There is no valid Google API Key for this application. Please visit the {0}Map section{1}.
+rendering.macro.map.apiKey.info=To use the map macro with Google Maps or Mapbox, you must get a [[Google API key&gt;&gt;{0}]] or a [[Mapbox access token&gt;&gt;{1}]].
+rendering.macro.map.apiKey.googlemaps.invalid=There is no valid Google API key for this macro. Please visit the [[Map Macro section&gt;&gt;{0}]].



I think I prefer the old version for translations (i.e. devs to take care of any kind of syntax: HTML, wiki). Imagine a contributor going to https://l10n.xwiki.org/projects/xwiki-contrib/map-macro/ in order to help. I think it would be easier to translate something like Please visit the {0}Map section{1} than Please visit the [[Map Macro section>>{0}]], or at least it would be cleaner.

acotiuga · 2018-07-06T12:27:22Z

src/main/resources/MapMacro/Code/Configuration.xml

  <content>{{velocity}}
 {{info}}
-  $services.localization.render('rendering.macro.map.apiKey.info', ['[[', '&gt;&gt;path:https://developers.google.com/maps/documentation/javascript/get-api-key]]'])
+  $services.localization.render('rendering.macro.map.apiKey.info', ['path:https://developers.google.com/maps/documentation/javascript/get-api-key', 'path:https://www.mapbox.com/account/access-tokens'])


See my comment on the translation, at the end of PR.

acotiuga · 2018-07-06T12:28:12Z

src/main/resources/XWiki/MapMacroTranslations.fr.xml

-  <date>1525702990000</date>
-  <contentUpdateDate>1525702967000</contentUpdateDate>
+  <date>1530017450000</date>
+  <contentUpdateDate>1530017450000</contentUpdateDate>


Same here, we avoid to commit date changes.

acotiuga · 2018-07-06T12:28:26Z

src/main/resources/XWiki/MapMacroTranslations.hr.xml

-  <date>1525702990000</date>
-  <contentUpdateDate>1525702967000</contentUpdateDate>
+  <date>1530017451000</date>
+  <contentUpdateDate>1530017451000</contentUpdateDate>


And here ...

acotiuga · 2018-07-06T12:28:43Z

src/main/resources/XWiki/MapMacroTranslations.sk.xml

-  <date>1525702990000</date>
-  <contentUpdateDate>1525702967000</contentUpdateDate>
+  <date>1530017450000</date>
+  <contentUpdateDate>1530017450000</contentUpdateDate>


…nStreetMap - fix formatting issue in MapMacro/Code/LeafletMap - reverted the l10n formatting for parameters - remove the Google Maps API key - restore original values of fields "creationDate", "date", "contentUpdateDate"

acotiuga · 2018-07-12T15:20:39Z

src/main/resources/MapMacro/Code/Configuration.xml

  <syntaxId>xwiki/2.1</syntaxId>
  <hidden>true</hidden>
-  <content>{{velocity}}
+  <content>{{velocity wiki="true"}}


This is not needed, as the default value for the wiki parameter is true. See: https://extensions.xwiki.org/xwiki/bin/view/Extension/Script%20Macro.

- remove unnecessary parameter "wiki=true" in the configuration

…nStreetMap - update version to "2.0-snapshot" to reflect a change in the default: from Google Maps to OSM

acotiuga · 2018-07-13T06:25:33Z

In the mean time, some small changes were done in this macro, by some other dev and this PR has conflicts that must be resolved.

…that these properties can either be in pixels or percent.

…ate wiki

the macro is installed

acotiuga · 2018-07-16T16:12:11Z

Thanks for your contribution!

MAP-16: Enhance the macro to support the display of maps based on Ope…

73a6256

…nStreetMap

slauriere requested a review from acotiuga June 27, 2018 13:40

slauriere requested a review from vmassol June 27, 2018 13:44

- add OSM geolocation

85011e1

- open OSM directions on click - make it possible to use % or px for width/height definition

acotiuga reviewed Jul 6, 2018

View reviewed changes

acotiuga reviewed Jul 12, 2018

View reviewed changes

slauriere added 2 commits July 12, 2018 17:24

MAP-16: Enhance the macro to support the display of maps based on OSM

856db3c

- remove unnecessary parameter "wiki=true" in the configuration

MAP-16: Enhance the macro to support the display of maps based on Ope…

1e89925

…nStreetMap - update version to "2.0-snapshot" to reflect a change in the default: from Google Maps to OSM

slauriere added 8 commits July 13, 2018 10:11

Merge branch 'master' into osm

7d40e6e

[misc] Update the list of contributors

d2aaf95

[misc] Update EN translation of width/height descriptions to reflect …

4b9af08

…that these properties can either be in pixels or percent.

[misc] Fix issue in retrieving XWiki.XWikiPreferences in the appropri…

ba4dada

…ate wiki

[misc] Update XML files properly formatted with "mvn xar:format"

7067352

[misc] Revert date/contentUpdate values to their original values

445f713

[misc] Make sure that the default unit for width/height is pixels (px)

1284aee

[misc] Update config URL: current wiki or main wiki depending on where

55fcfea

the macro is installed

acotiuga merged commit 5fdd395 into master Jul 16, 2018

MAP-16: Enhance the macro to support the display of maps based on OpenStreetMap #9

MAP-16: Enhance the macro to support the display of maps based on OpenStreetMap #9

Uh oh!

Conversation

slauriere commented Jun 27, 2018

Uh oh!

slauriere commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acotiuga Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acotiuga commented Jul 13, 2018

Uh oh!

acotiuga commented Jul 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

slauriere commented Jun 27, 2018 •

edited

Loading

acotiuga Jul 6, 2018 •

edited

Loading