Skip to content

Vector map, geocentroid, tilemap bug fixes#10824

Closed
thomasneirynck wants to merge 41 commits intoelastic:masterfrom
thomasneirynck:feature/map
Closed

Vector map, geocentroid, tilemap bug fixes#10824
thomasneirynck wants to merge 41 commits intoelastic:masterfrom
thomasneirynck:feature/map

Conversation

@thomasneirynck
Copy link
Copy Markdown
Contributor

@thomasneirynck thomasneirynck commented Mar 20, 2017

Reopening Vector Map against master iso. feature branch (cf. #10448)


The scope of this PR is the introduction of the vector map, which includes some map enhancements that will benefit both TileMap and VectorMap.


Feedback:

Todos:

  • make vector maps configurable in .yml
  • add labeling to choropleth maps (just tooltips now, would need ot use L.label plugin due to dependency on leaflet 0.7)
  • update test code
  • provide better default data (add more join-fields)
  • multi tile-map, which was deprecated, is now fully disabled. Perhaps we should re-evaluate this.

@thomasneirynck thomasneirynck mentioned this pull request Mar 20, 2017
14 tasks
@alexfrancoeur
Copy link
Copy Markdown

@thomasneirynck some quick feedback from vector maps below

Since we can only define one metric, can we automatically expand? I see no reason to have the metric options collapsed by default. Users can quickly glance over these things if it's not in their face
screen shot 2017-03-20 at 6 56 33 pm

If I leave the visualization and return, I enter a zoomed out screen. I've been able to reproduce (with and without refreshing Kibana) multiple times
screen shot 2017-03-20 at 7 00 39 pm

Will add more tomorrow, need to find a better dataset to use.

@thomasneirynck
Copy link
Copy Markdown
Contributor Author

@alexfrancoeur thanks for the quick feedback already. the zooming is a known issue, this needs to be fixed.

agreed on the auto-expand

@alexfrancoeur
Copy link
Copy Markdown

@thomasneirynck alright, have some data and up and running now.

Good that we agree on auto-expanding. Both configurations only have one option - may as well save some clicks :-) If we auto-expand here, I believe we'd need to on tile map as well (and possibly other visualizations). This should really be consistent for configurations with only one option available. It may be a larger task than vector maps.

You have already stated that this is a bug, but I thought I'd share what it now looks like with data as well. I am also seeing this in tile maps as well.

screen shot 2017-03-21 at 12 05 57 pm

Is there any error handling we can provide if the field provided does not match the two letter abbreviation or state / country name?

If I switch between World Countries and States and hit play, I lose the tooltip when hovering over a country. I can't seem to get it back by toggling any of the other configurations either. There also seems to be a bit of lag when switching between the two in the dropdown without hitting play. Are we loading anything behind the scenes like a geojson file?

mar-21-2017 12-20-53

Also, as you can see in the above GIF the zoom is a bit touchy. It could just be my mouse (magic mouse). I'll have to try on my laptop as well. I only mention because it looks as if you enhanced the zooming capabilities.

I don't believe we have much control over the colors but white is fairly difficult to see. Maybe we could provide an option to configure the outline color of the vector shape?

screen shot 2017-03-21 at 12 24 58 pm

I got this error when attempting to use geo centroid for a second time. Not sure why it worked with first time. Also - is the geo centroid aggregation meant to be used in tile maps? I see it as an option for other visualizations but not the tile map.

screen shot 2017-03-21 at 12 36 22 pm

Hope this helps - will ping you if I find anything else.

@tbragin tbragin added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Mar 21, 2017
@thomasneirynck
Copy link
Copy Markdown
Contributor Author

thanks @alexfrancoeur, that's great.

  • for auto-expand, I agree, we could do this in a separate PR, since this will effect other visualizations that only have a single bucket-type. For example, tag-cloud should auto-expand as well.

  • Is there any error handling we can provide if the field provided does not match the two letter abbreviation or state / country name?

This is a tough one. How do we know this is because of an error, or just an artifact of the data? I would suggest we show a warning when there is data from ES that cannot be joined to the vector map. I'll make this part of this PR.

  • the disappearing tooltip is a bug. This needs to be fixed.

  • There is somewhat of a lag when we switch the data-files, as the new vector map needs to be downloaded (only once though, not multiple times). One thing is that the states-map is larger than it needs to be. I will trim both these files to a smaller default size. Another thing I can add is to show a small spinner or use the kibana busy-bar to show something is happening.

  • the zooming on the vector-map is as good as it gets from Leaflet. At some point we'll want to move to a later version but we have a few blockers there (Upgrade to Leaflet 1.0.3 #10276). There's some informal chatter about using a different library altogether but that is out of scope now.

  • agreed on the color ranges. I'm reusing the same ones from the Heatmap-viz, but they don't work well with the background. I'll improve those.

  • the geo-centroid is NOT meant to be used in vector maps. This inclusion is a bug and needs to be fixed.

@tbragin tbragin changed the title Vector map Vector map, geocentroid, tiiemap bug fixes Mar 22, 2017
@tbragin tbragin added the review label Mar 22, 2017
@tbragin tbragin changed the title Vector map, geocentroid, tiiemap bug fixes Vector map, geocentroid, tilemap bug fixes Mar 22, 2017
@tbragin
Copy link
Copy Markdown
Contributor

tbragin commented Mar 22, 2017

@thomasneirynck Great work!

In addition to geo-centroid aggregations, it appears that pipeline aggs can't be use with Vector Map, so we should remove those from the picklist as well
screen shot 2017-03-22 at 5 51 22 am

Also, when i change the aggregation back, the red message tends to hang around... this may be more of an issue that is already there
screen shot 2017-03-22 at 5 52 15 am

@tbragin
Copy link
Copy Markdown
Contributor

tbragin commented Mar 22, 2017

@thomasneirynck We previously talked about a data label option for this visualization type, have you given it more thoughts? I don't see this option in the PR I just pulled down, even though the task at the top is checked off.

@tbragin
Copy link
Copy Markdown
Contributor

tbragin commented Mar 22, 2017

@thomasneirynck Regarding the geocentroid option in the Tilemap, could we format the option differently?

  • First, we need a space after it, i think -- currently it blends with the "Custom label" -- almost looks like the latter is a continuation of the previous.
  • Second, I don't like that we use the word "center" twice in that option and the name is so long and cryptic. I also think we should mention "geocentroid" in there for the more technical audience (our vis builder already uses ES terminology all over the place). How about naming it "Place markers off grid (use geocentroid)"? I'm open to other suggestions too.

Current
screen shot 2017-03-22 at 6 13 56 am

Proposed
screen shot 2017-03-22 at 6 21 59 am

@tbragin
Copy link
Copy Markdown
Contributor

tbragin commented Mar 22, 2017

@thomasneirynck I saw some errors in the console, not sure if you already know of them?

UPDATE: Never mind, it's a chrome extension.

Uncaught TypeError: Cannot read property 'getElementsByClassName' of undefined
    at injectCSVExportButton (chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/inject.js:168:63)
    at HTMLDivElement.<anonymous> (chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/inject.js:28:11)
    at Object.callCallbacks (chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/lib/arrive.js:40:23)
    at chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/lib/arrive.js:285:15
    at Array.forEach (native)
    at MutationObserver.onArriveMutation (chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/lib/arrive.js:270:17)
    at MutationObserver.<anonymous> (chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/lib/arrive.js:158:20)

@tbragin
Copy link
Copy Markdown
Contributor

tbragin commented Mar 22, 2017

@thomasneirynck I had significant slowness drilling in on the logs getting started dataset, going from the previous zoom level into this one and beyond. This is specifically bad when i turn on geocentroid as an option. Could someone double-check it's not just me? Slowness was so bad, i thought things were broken. Have we "load tested" this PR on the tilemap, meaning used it heavily with a large datasets, panning and zooming, etc..?

screen shot 2017-03-22 at 6 28 34 am

@kobelb
Copy link
Copy Markdown
Contributor

kobelb commented Mar 22, 2017

Uncaught TypeError: Cannot read property 'getElementsByClassName' of undefined
at injectCSVExportButton (chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/inject.js:168:63)
at HTMLDivElement. (chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/inject.js:28:11)
at Object.callCallbacks (chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/lib/arrive.js:40:23)
at chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/lib/arrive.js:285:15
at Array.forEach (native)
at MutationObserver.onArriveMutation (chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/lib/arrive.js:270:17)
at MutationObserver. (chrome-extension://kjkjddcjojneaeeppobfolgojhohbpjn/src/inject/lib/arrive.js:158:20)

@tbragin This looks like a chrome extension is throwing this error, probably not related to the Tilemap changes

@tbragin
Copy link
Copy Markdown
Contributor

tbragin commented Mar 22, 2017

Ugh yeah, i forgot i had that in there... Thanks @kobelb :)

@thomasneirynck
Copy link
Copy Markdown
Contributor Author

thomasneirynck commented Mar 22, 2017

@tanya i think I identified the slow issue with the sample dataset for the Tilemap. I will work on the fix. I introduced a regression when adding some of the other additions.

[EDIT] this is fixed now

@thomasneirynck
Copy link
Copy Markdown
Contributor Author

@tbragin with regard to labeling the shapes: The labeling required an extra plugin to Leaflet, and I'm hesitant to introduce dependencies-to-dependencies. Especially since our reliance on the heatmap-plugin is currently preventing us from upgrading to the latest Leaflet version. But I'll take a second look, and let you know.

1 similar comment
@thomasneirynck
Copy link
Copy Markdown
Contributor Author

@tbragin with regard to labeling the shapes: The labeling required an extra plugin to Leaflet, and I'm hesitant to introduce dependencies-to-dependencies. Especially since our reliance on the heatmap-plugin is currently preventing us from upgrading to the latest Leaflet version. But I'll take a second look, and let you know.

@thomasneirynck
Copy link
Copy Markdown
Contributor Author

Separated out geocentroid/tilemap fixed in #10871. This is to resolve blocker quicker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants