Fix/map zoom #6835
Fix/map zoom #6835
Conversation
| mapMoveEnd: function (event) { | ||
| mapMoveEnd: function (event, uiState) { | ||
| const mapPrecision = zoomPrecision[event.zoom]; | ||
| uiState.set('vis.params.mapCenter', event.center); |
There was a problem hiding this comment.
Why vis.params? these can just be 'mapCenter', 'mapZoom', and 'mapPrecision' I think. Otherwise it's confusing.
| }, | ||
| deserialize: getPrecision, | ||
| write: function (aggConfig, output) { | ||
| output.params.precision = getPrecision(aggConfig.params.precision); | ||
| let currZoom = aggConfig.vis.params.mapZoom; |
There was a problem hiding this comment.
Not a fan of this function. I have to figure out whether the aggConfig, vis, or uiState has the proper zoom, to base the precision off of. This happens differently whether you load up the dashboard with the same vis, refresh on the vis page, or set it by zooming or panning on the map
There was a problem hiding this comment.
I think the proper solution is to fix the dashboard and ensure the vis gets it uiState on the dashboard.
|
@rashidkpc gave me a bit of help along the way. Not a fan of all the extra stuff in that pull though. |
| write: function (aggConfig, output) { | ||
| output.params.precision = getPrecision(aggConfig.params.precision); | ||
| let currZoom = null; | ||
| if (aggConfig.params.mapZoom || aggConfig.vis.hasUiState()) { // First iteration |
There was a problem hiding this comment.
I'm not a fan of all of this aggconfig.params business, this IMO be removable
panda01
force-pushed
the
fix/map-zoom
branch
4 times, most recently
from
0700a49 to
bd4b5c6
Compare
|
I have addressed the aforementioned issues, and simplified this a bit. |
|
Looks good functionally, and I believe the edge case issues have all been corrected. LGTM. |
| } | ||
| mapMoveEnd: function (event, uiState) { | ||
| uiState.set('mapCenter', event.center); | ||
| uiState.set('mapZoom', event.zoom); |
There was a problem hiding this comment.
I'm not sure its necessary to set the mapZoom here. Just setting the mapCenter should be sufficient when moving the map, no?
There was a problem hiding this comment.
I thought it was because during the auto fit it didn't call the zoom function, but after testing it seems I'm mistaken. I'll take it out.
|
Looks good functionally. Just left one minor comment on the code: https://github.com/elastic/kibana/pull/6835/files#r64831202. |
|
@ycombinator I addressed your concerns. If you could give me the final acronym of approval I'll go ahead and merge. |
|
LGTM. |
The ability to create filters by clicking on visualizations was broken between 5.0 alpha3 and alpha4. The root cause was the addition of a new parameter being sent to all visualization event handlers, which was added to support persistence of tilemap position and zoom level. The filter_bar_click_handler (which enables the vis filtering behavior) already expected a different kind of second parameter, so the addition of this new parameter threw off the click handler's internal logic. It turns out that the new parameter is unnecessary, because the object it's passing is already available on the event object. So to fix this, I removed the new parameter from the emitter and updated the tilemap handlers to grab the object off of the event. Fixes: elastic#7501 Related: elastic#6835 (added the new param)
The ability to create filters by clicking on visualizations was broken between 5.0 alpha3 and alpha4. The root cause was the addition of a new parameter being sent to all visualization event handlers, which was added to support persistence of tilemap position and zoom level. The filter_bar_click_handler (which enables the vis filtering behavior) already expected a different kind of second parameter, so the addition of this new parameter threw off the click handler's internal logic. It turns out that the new parameter is unnecessary, because the object it's passing is already available on the event object. So to fix this, I removed the new parameter from the emitter and updated the tilemap handlers to grab the object off of the event. Fixes: elastic#7501 Related: elastic#6835 (added the new param) Former-commit-id: 20cb910
This fixes map zoom persistence issues.
Couples more the uiState with the vis.
Adds uiState persistence values for
mapZoom, andmapCenter.In short makes it so you can save maps that load already at a specific location, and so that when you reload a map it stays at the same place.
Closes #1442.