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] auto-fit to data bounds #72129

Merged
merged 16 commits into from
Jul 20, 2020
Merged

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jul 16, 2020

Fixes #2054, #32738, #53831.

Continuation of #66452

This PR adds a new map setting that will auto fit the map to data bounds.

Screen Shot 2020-05-13 at 12 31 09 PM

@nreese nreese requested a review from a team as a code owner July 16, 2020 18:36
@nreese nreese added release_note:enhancement [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.10.0 v8.0.0 WIP Work in progress labels Jul 16, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@jsanz
Copy link
Member

jsanz commented Jul 17, 2020

Tested locally on Firefox and Chromium and works perfectly for geo_point document, geo_shape, and agg layers (grid, cluster, and heatmap) layers. This is exciting!!

With EMS boundaries the behavior is inconsistent though, sometimes the view fits to the bounds, sometimes it does not. Feels like some issue with the synchronicity of events or something like that (very wild guess).

Finally, from the usability point of view, I think I would prefer a bit of a buffer between the data and the map borders, so instead of going exactly to the bounds, extending it a small fraction like 10% of the screen size or even less.

So instead of this

image

Zooming out a bit like this

image

@nreese
Copy link
Contributor Author

nreese commented Jul 17, 2020

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Jul 17, 2020

Finally, from the usability point of view, I think I would prefer a bit of a buffer between the data and the map borders, so instead of going exactly to the bounds, extending it a small fraction like 10% of the screen size or even less.

good suggestion, I added 10% scaling factor.

@kmartastic
Copy link
Contributor

I think this behavior should be ON by default. Objections?

@jsanz
Copy link
Member

jsanz commented Jul 17, 2020

I think this behavior should be ON by default. Objections?

I'm not 100% sure if this could be confusing for users since the Map Settings panel is pretty new. Would make sense to change somehow the Fit to data icon when this setting is enabled to visually highlight it and help users understand what's going on? Not strong on this either way.

@nreese
Copy link
Contributor Author

nreese commented Jul 17, 2020

With EMS boundaries the behavior is inconsistent though, sometimes the view fits to the bounds, sometimes it does not. Feels like some issue with the synchronicity of events or something like that (very wild guess).

There was a problem with join layers and auto fit to bounds. The original approach ran fit to bounds before fetching data for all layers. For non-join layers, this is preferred because you want the new bounds before fetching data. However, this breaks join layers because join layers live on the client so they need re-freshed data before you can run fit to bounds.

I have updated the logic to syncData for join layers prior to running fit to bounds. This fixes the issue you saw with EMS boundary layers.

@nreese
Copy link
Contributor Author

nreese commented Jul 17, 2020

I think this behavior should be ON by default. Objections?

moving discussion to #72340

Would make sense to change somehow the Fit to data icon when this setting is enabled to visually highlight it and help users understand what's going on?

moving discussion to #72341

Both are great questions. I would like to move them into there own threads and avoid discussion in this PR. Once the basic feature is merged, we can discuss these as needed but I do not want to block/distract the basic feature for these non-essential elements.

@nreese
Copy link
Contributor Author

nreese commented Jul 17, 2020

@elasticmachine merge upstream

Copy link
Member

@nickpeihl nickpeihl left a comment

Choose a reason for hiding this comment

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

lgtm! I love it.

tested in Chrome.

Copy link
Member

@jsanz jsanz left a comment

Choose a reason for hiding this comment

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

tested again with EMS boundaries and working fine, thanks for opening the new issues for further discussion 👍

@nreese
Copy link
Contributor Author

nreese commented Jul 20, 2020

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Jul 20, 2020

What should the interaction be with the initial map view and auto fit to bounds? Should the map auto fit to bounds when first opened? Or respect the initial view setting?

@kmartastic
Copy link
Contributor

@nreese Can you explain the initial view setting to me? Is this a user setting?

@nreese
Copy link
Contributor Author

nreese commented Jul 20, 2020

Can you explain the initial view setting to me? Is this a user setting?

Users can configure the initial location of the map to be the location at save, a fixed location, or a browser location. Should auto fit to bounds effect this choice? When auto fit to bounds is enabled, should the initial map location use this setting or the fit to the data bounds?

Screen Shot 2020-07-20 at 9 36 06 AM

@nreese nreese removed the WIP Work in progress label Jul 20, 2020
@kmartastic
Copy link
Contributor

Can you explain the initial view setting to me? Is this a user setting?

Users can configure the initial location of the map to be the location at save, a fixed location, or a browser location. Should auto fit to bounds effect this choice? When auto fit to bounds is enabled, should the initial map location use this setting or the fit to the data bounds?

Screen Shot 2020-07-20 at 9 36 06 AM

We should honor this setting on initial load of the map.

@nreese
Copy link
Contributor Author

nreese commented Jul 20, 2020

We should honor this setting on initial load of the map.

Sounds like the consensus is that "auto fit to bounds" should be an independent setting from "initial map location". I like this choice because it gives users the greatest flexibly to configure their mapping experience. I have opened #72480 to track adding "fit to data bounds" under "initial map location" so users have the ability to open maps to data bounds.

Since this PR preserves the behavior of initial map location, it can merge as is and #72480 will address initial map location with regards to fit to bounds

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
maps 3.8MB +2.3KB 3.8MB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@nreese nreese merged commit 9947c67 into elastic:master Jul 20, 2020
nreese added a commit to nreese/kibana that referenced this pull request Jul 20, 2020
* [Maps] auto-fit to data bounds

* update jest snapshot

* add buffer to fit to bounds

* sync join layers prior to fitting to bounds

* clean-up comment

* better names

* fix tslint errors

* update functional test expect

* add functional tests

* clean-up

* change test run location

* fix test expect

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
nreese added a commit that referenced this pull request Jul 20, 2020
* [Maps] auto-fit to data bounds

* update jest snapshot

* add buffer to fit to bounds

* sync join layers prior to fitting to bounds

* clean-up comment

* better names

* fix tslint errors

* update functional test expect

* add functional tests

* clean-up

* change test run location

* fix test expect

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 21, 2020
* master: (28 commits)
  allow some env settings for ingest manager (elastic#72544)
  Add inspector for VEGA (elastic#70941)
  chore(NA): fix grunt task for test:coverage (elastic#72539)
  Archive e2e test results in ES (elastic#72575)
  preserve 401 errors from new es client (elastic#71248)
  [SIEM][Detections] Updates text for severity and risk_score overrides  (elastic#72244)
  fixing error occurences tooltip (elastic#72425)
  use KibanaClient interface instead of Client for new client interface (elastic#72388)
  [APM] Handle ML errors (elastic#72316)
  [Discover] Improve histogram tests (elastic#72235)
  [ftr/webdriver] retry on all errors, use Rx so that timers are canceled (elastic#72540)
  [pre-req] Move .storybook to storybook; standardize files (elastic#72384)
  [Security_Solution][Resolver][Bug]: Restore breadcrumb background (elastic#72538)
  [ML] Fix annotation detector linking & delayed_data(0) (elastic#72468)
  [Security Solution][Exceptions] - Make esTypes and subType available to index patterns (elastic#72336)
  [SIEM] Uses faster wait from testing-library and removes duplicate older wait idiom (elastic#72509)
  Fix long combo box items breaking out of flex item width (elastic#72512)
  [pipeline/commitStatus] update commit status in baseline-capture job (elastic#72366)
  [Security Solution][Resolver] Update the resolver element ref on scroll events if the position of the element has changed within the page (elastic#72461)
  [Maps] auto-fit to data bounds (elastic#72129)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coordinate Maps: auto fit data bounds
6 participants