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

[Maps] use geo-tile aggregation instead of geohash precision #29776

Merged
merged 26 commits into from
Feb 5, 2019

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Jan 31, 2019

Closes #28485
Replaces #29477

@thomasneirynck thomasneirynck added bug Fixes for quality problems that affect the customer experience v7.0.0 [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation labels Jan 31, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@thomasneirynck thomasneirynck removed the bug Fixes for quality problems that affect the customer experience label Jan 31, 2019
@thomasneirynck thomasneirynck changed the title [Maps] use geo-tile aggregation iso geohash precision [Maps] use geo-tile aggregation instead of geohash precision Jan 31, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@thomasneirynck thomasneirynck added the WIP Work in progress label Feb 1, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@thomasneirynck
Copy link
Contributor Author

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@thomasneirynck
Copy link
Contributor Author

jenkins, test this

@thomasneirynck thomasneirynck removed the WIP Work in progress label Feb 5, 2019
@thomasneirynck thomasneirynck requested a review from nreese February 5, 2019 14:03
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

return column.aggConfig.type.type === 'metrics'
&& column.aggConfig.type.dslName !== 'geo_centroid';
});
const geocentroidColumn = table.columns.find(column => column.aggConfig.type.dslName === 'geo_centroid');
Copy link
Contributor

Choose a reason for hiding this comment

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

Check that geocentroidColumn and throw an error if does not exist since there is not longer a check before geocentroidColumn.id

@@ -36,20 +37,28 @@ export default function ({ getPageObjects, getService }) {
beforeTimestamp = await getRequestTimestamp();
});

it('should not rerequest when zoom changes do not cause geohash precision to change', async () => {
await PageObjects.maps.setView(DATA_CENTER_LAT, DATA_CENTER_LON, 2);
it('should not rerequest when pan changes do not cause geotile_grid precision to change', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be should not rerequest when pan changes do not move map view area outside of buffer

@nreese
Copy link
Contributor

nreese commented Feb 5, 2019

One more thing. The test map saved object is still called geohashgrid heatmap example and geohashgrid vector grid example. Maybe these should be renamed to something more generic, like geo grid heatmap example and geo grid vector grid example. You can change the title in the data.json file


_loadUrl = async () => {
const tilemap = await getKibanaTileMap();
if (this._isMounted) {
this.setState({
url: tilemap.url
});
this.props.previewTilemap(this.props.url);
this.props.previewTilemap(this.state.url);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this change snuck in

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

This is great. geotile_grid gives a much smoother gridding system while zooming

lgtm
code review, tested changes in chrome

@thomasneirynck thomasneirynck merged commit 3ccf679 into master Feb 5, 2019
@spalger spalger deleted the feature/geotile_grid branch November 8, 2019 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants