Region map should respect saved center and zoom #12883

thomasneirynck · 2017-07-14T22:29:18Z

Release Note: The location of the map is now stored correctly when saving a Region Map.

ppisljar · 2017-07-18T16:25:17Z

src/core_plugins/region_map/public/region_map_controller.js

+
+    const saveCurrentLocation = () => {
+      //to ensure we dirty the state of the viz. (not sure why this is required. Tilemap visualization does not require it)
+      $scope.vis.params.mapZoom = kibanaMap.getZoomLevel();


this should be stored to uiState as it is modified on dashboard (without editor)

oh i see its already stored in uiState ... why do you need this then ?

you're right. I don't know, maybe I got in a weird state....

ppisljar · 2017-07-18T16:26:12Z

src/core_plugins/region_map/public/region_map_controller.js

+    const uiState = $scope.vis.getUiState();
+    const zoomFromUiState = parseInt(uiState.get('mapZoom'));
+    const centerFromUIState = uiState.get('mapCenter');
+    options.zoom = !isNaN(zoomFromUiState) ? zoomFromUiState : $scope.vis.type.visConfig.defaults.mapZoom;


if you store the center and zoom in ui state this won't be needed

if its not in uiState you should just fallback to a fixed setting, no need to define it on visConfig

const defaultOptions = { zoom: 2, mapCenter: [0, 0] }; const options = _.defaultsDeep({}, { zoom: parseInt(uiState.get('mapZoom'), mapCenter: uiState.get('mapCenter'), ...minMaxZoom }, defaultOptions);

in other visualizations we do something like this ...

ppisljar · 2017-07-18T16:27:21Z

src/core_plugins/region_map/public/region_map_vis.js

-        selectedJoinField: selectedJoinField
+        selectedJoinField: selectedJoinField,
+        mapZoom: 2,
+        mapCenter: [0, 0]


if you put it in uiState you'll need to init it when you create your chart there (if its not yet present)

ppisljar

works nice ... some minor comments

thomasneirynck · 2017-07-18T20:03:55Z

you're right, saving in ui-state is sufficient. not sure why I missed it. besides, it struck me that we didn't need it in tilemap either. Removed the crufty statement

ppisljar

i think you missed my comment regarding storing things that are not configurable in editor in vis.params ...
i think you should not be storing map center and map zoom there, as this are uiState settings (right now they are both present in URL twice for this reason)

above in the comment i also suggested a diff approach (something all the other visualizations are doing)

thomasneirynck · 2017-07-27T03:35:44Z

thx, sorry I missed that improvement. Updated with latest commit.

ppisljar

LGTM

stacey-gammon · 2017-07-27T15:54:59Z

It looks like one you edit the location in dashboard, it will no longer reset to the new saved map center value from the visualization. I guess this is expected, similar to the issue/"feature" with saved searches and overriding default columns/sort order (#9523)

I suppose this is an improvement then the prior implementation, but we may want to think more about how to sync the dashboard panel overridden state back with the original visualization.

thomasneirynck · 2017-07-27T19:03:00Z

@stacey-gammon, yeah, this PR is not supposed to fix that.This one needs to fix primarily the original issue that Region Map does not save the current zoom/center when pressing the save-button.

but agreed that maintaning dashboard-state will need some resolution at some point. Not sure what the right way forward on that one is though...

Required manual edits to backport.

thomasneirynck · 2017-07-27T23:17:41Z

Backports:
5.5: #13187
5.6/5.x: #13171
6.0: #13170
6.x: #13169

ppisljar · 2017-07-28T06:52:55Z

regarding the dashboard comment above:
doesn't this behave the same was as the colors (should) ?

so in visualization you can define your center (or colors). when you put this viz on dashboard it will show as you set it. however you can now do additional changes (move it, change some colors) on the dashboard and save this. now you share this dashboard with somebody, he is gonna see the version you saved with dashboard. he can still move around and change the colors even. however the last colors and position are only stored for him specifically (until he hard refreshes ...)

or does center location behave differently somehow ?

stacey-gammon · 2017-07-28T13:43:12Z

Kind of - the "until he hard refreshes" part depends. If the dashboard was saved after the map center/color changes were made, then it's stuck with the custom colors and map positions unless the visualization is removed and re-added. That's the only way to sync back to the original visualization.

But you are right, map center behaves just like the other overridable panel states, like color. It's just a confusing interaction IMO, that you can override state and never get it back. Especially when some of these actions can happen in view mode, then the user can go into edit mode, make an unrelated change, save, and now they are also stuck with the changes they made in view mode.

But this is a whole different can of worms, something the sharing team will have to figure out. :)

thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v5.5.1 v5.6.0 v6.0.0 review labels Jul 14, 2017

thomasneirynck requested a review from ppisljar July 18, 2017 15:04

ppisljar reviewed Jul 18, 2017

View reviewed changes

ppisljar requested changes Jul 18, 2017

View reviewed changes

thomasneirynck requested a review from ppisljar July 19, 2017 19:35

thomasneirynck added the release_note:fix label Jul 19, 2017

ppisljar requested changes Jul 20, 2017

View reviewed changes

epixa added v5.5.2 and removed v5.5.1 labels Jul 21, 2017

thomasneirynck requested a review from ppisljar July 27, 2017 03:34

ppisljar approved these changes Jul 27, 2017

View reviewed changes

thomasneirynck added 4 commits July 27, 2017 09:09

save zoomcenter regionmap

2a12cec

reorganize

d70ae78

saving in uistate is sufficient

b67605b

do not store constant defaults in params

efc2d00

thomasneirynck force-pushed the fix/save_zoomcenter_regionmap branch from a98ea85 to efc2d00 Compare July 27, 2017 13:09

stacey-gammon approved these changes Jul 27, 2017

View reviewed changes

thomasneirynck merged commit 5e9a4ec into elastic:master Jul 27, 2017

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 27, 2017

Region map should respect saved center and zoom (elastic#12883)

6c47ca3

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 27, 2017

Region map should respect saved center and zoom (elastic#12883)

b466fd5

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 27, 2017

Region map should respect saved center and zoom (elastic#12883)

e7c5af1

Required manual edits to backport.

thomasneirynck added a commit that referenced this pull request Jul 28, 2017

Region map should respect saved center and zoom (#12883) (#13169)

4d93c74

thomasneirynck added a commit that referenced this pull request Jul 28, 2017

Region map should respect saved center and zoom (#12883) (#13170)

c76a02d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Region map should respect saved center and zoom #12883

Region map should respect saved center and zoom #12883

thomasneirynck commented Jul 14, 2017 •

edited

Loading

ppisljar Jul 18, 2017

ppisljar Jul 18, 2017

thomasneirynck Jul 18, 2017

ppisljar Jul 18, 2017

ppisljar Jul 18, 2017

ppisljar Jul 19, 2017 •

edited

Loading

ppisljar Jul 18, 2017

ppisljar left a comment

thomasneirynck commented Jul 18, 2017 •

edited

Loading

ppisljar left a comment

thomasneirynck commented Jul 27, 2017

ppisljar left a comment

stacey-gammon commented Jul 27, 2017

thomasneirynck commented Jul 27, 2017

thomasneirynck commented Jul 27, 2017 •

edited

Loading

ppisljar commented Jul 28, 2017

stacey-gammon commented Jul 28, 2017

Region map should respect saved center and zoom #12883

Region map should respect saved center and zoom #12883

Conversation

thomasneirynck commented Jul 14, 2017 • edited Loading

ppisljar Jul 18, 2017

Choose a reason for hiding this comment

ppisljar Jul 18, 2017

Choose a reason for hiding this comment

thomasneirynck Jul 18, 2017

Choose a reason for hiding this comment

ppisljar Jul 18, 2017

Choose a reason for hiding this comment

ppisljar Jul 18, 2017

Choose a reason for hiding this comment

ppisljar Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

ppisljar Jul 18, 2017

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

thomasneirynck commented Jul 18, 2017 • edited Loading

ppisljar left a comment

Choose a reason for hiding this comment

thomasneirynck commented Jul 27, 2017

ppisljar left a comment

Choose a reason for hiding this comment

stacey-gammon commented Jul 27, 2017

thomasneirynck commented Jul 27, 2017

thomasneirynck commented Jul 27, 2017 • edited Loading

ppisljar commented Jul 28, 2017

stacey-gammon commented Jul 28, 2017

thomasneirynck commented Jul 14, 2017 •

edited

Loading

ppisljar Jul 19, 2017 •

edited

Loading

thomasneirynck commented Jul 18, 2017 •

edited

Loading

thomasneirynck commented Jul 27, 2017 •

edited

Loading