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

Display regionmap attribution #12647

Merged
merged 8 commits into from
Jul 13, 2017

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Jul 3, 2017

Closes #12518, #10851

This PR works around some of the kinks in the manifest

  • attribution is single string (perhaps this should be an array)
  • attribution delimiter is | in tiles-manifest, but , in vector manifest. Should align this (or use arrays).
  • another options is to prohibit duplicates in attribution. This would mean moving the Elastic Maps attribution to a top level (e.g. the catalogue manifest).

@thomasneirynck thomasneirynck requested a review from ppisljar July 4, 2017 13:22
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

some initial comments and mostly questions ...

@@ -165,6 +167,7 @@ export default class ChoroplethLayer extends KibanaMapLayer {

}


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 you can revert changes to this file, it just adds some newlines, unnecessary noise

@@ -54,7 +54,8 @@ module.controller('KbnRegionMapController', function ($scope, $element, Private,
return;
}

updateChoroplethLayer($scope.vis.params.selectedLayer.url);
console.log($scope.vis.params.selectedLayer);
Copy link
Member

Choose a reason for hiding this comment

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

how can you commit console.log statements ? pre-commit step doesn't run for you ? (i tried doing that several times but was unable to do so ... )

@@ -91,6 +92,7 @@ module.controller('KbnRegionMapController', function ($scope, $element, Private,
const tmsSettings = await serviceSettings.getTMSService();
const minMaxZoom = tmsSettings.getMinMaxZoom(false);
kibanaMap = new KibanaMap($element[0], minMaxZoom);
window._kibanaMap = kibanaMap;
Copy link
Member

Choose a reason for hiding this comment

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

should we really be storing this to window object ? sounds wrong

attribution: options.attribution
subdomains: options.subdomains || []
//,
// attribution: options.attribution
Copy link
Member

Choose a reason for hiding this comment

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

?

});
}

_getWMSBaseLayer(options) {
const wmsOptions = {
attribution: options.attribution || '',
// attribution: options.attribution || '',
Copy link
Member

Choose a reason for hiding this comment

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

remove commented out parts if they are no longer necesarry

updateAttributions(attributionString) {
attributionString = attributionString || '';
let attributions = attributionString.split('|');
if (attributions.length === 1) {//temp work-around due to inconsistency in manifests
Copy link
Member

Choose a reason for hiding this comment

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

how much work is involved in changing the manifests ?

@thomasneirynck thomasneirynck changed the title display regionmap attribution [WIP] display regionmap attribution Jul 5, 2017
@thomasneirynck thomasneirynck changed the title [WIP] display regionmap attribution Display regionmap attribution Jul 6, 2017
@thomasneirynck
Copy link
Contributor Author

This required changes to deal with handling duplicates on removal correctly.

@thomasneirynck thomasneirynck requested a review from ppisljar July 6, 2017 21:15
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

can attribution be provided for custom geo shapes ? (thru kibana.yml ? )

@thomasneirynck
Copy link
Contributor Author

@ppisljar no, but that is something that probably should be added. I'll do that in this PR.

@thomasneirynck thomasneirynck added Feature:Region Map Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:enhancement v5.5.1 v6.0.0 labels Jul 11, 2017
@thomasneirynck thomasneirynck requested a review from bhavyarm July 13, 2017 19:25
Copy link
Contributor

@bhavyarm bhavyarm left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasneirynck thomasneirynck merged commit 58ef02c into elastic:master Jul 13, 2017
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 13, 2017
EDIT: This is a manual backport, and required resolving merge conflicts when backporting.

This adds the attribution strings of vector data files to the map. It also enables the map to allow for individual attribution strings from each individual layer, so attribution gets updated correctly when adding/removing layers.
@thomasneirynck
Copy link
Contributor Author

thomasneirynck commented Jul 13, 2017

Backports:
5.x: #12845
5.5: #12849

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 14, 2017
EDIT: This is a manual backport, and required resolving merge conflicts when backporting.

This adds the attribution strings of vector data files to the map. It also enables the map to allow for individual attribution strings from each individual layer, so attribution gets updated correctly when adding/removing layers.
thomasneirynck added a commit that referenced this pull request Jul 14, 2017
EDIT: This is a manual backport, and required resolving merge conflicts when backporting.

This adds the attribution strings of vector data files to the map. It also enables the map to allow for individual attribution strings from each individual layer, so attribution gets updated correctly when adding/removing layers.
thomasneirynck added a commit that referenced this pull request Jul 14, 2017
EDIT: This is a manual backport, and required resolving merge conflicts when backporting.

This adds the attribution strings of vector data files to the map. It also enables the map to allow for individual attribution strings from each individual layer, so attribution gets updated correctly when adding/removing layers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Region Map Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:enhancement review v5.5.1 v5.6.0 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants