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] Added options to disable zoom, hide tool tips, widgets/overlays in embeddable maps #50663

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Nov 14, 2019

Fixes: #49871

Summary

To achieve the skimmed down version to be used in Uptime app,
Following configurable options are added into embeddable maps API

  1. Disable Double click zooming
  2. Disable zoom(hide zoom control)
  3. Hide widget overlays
  4. Hide Panel header

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@shahzad31 shahzad31 requested a review from a team as a code owner November 14, 2019 15:18
@nreese nreese added the [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation label Nov 14, 2019
@elasticmachine
Copy link
Contributor

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

@nreese nreese added the v7.6.0 label Nov 14, 2019
@shahzad31 shahzad31 added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Nov 14, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@shahzad31 shahzad31 removed the request for review from a team November 14, 2019 15:47
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor

nreese commented Nov 14, 2019

Thanks for opening a PR for #49871

Users can still zoom in by holding shift and dragging over an area. You may want to set interactive: false when initializing the map.

This PR introduces a lot of variables for the embeddable input:

  • disableZoom
  • disableTooltipControl
  • hideToolbarOverlay
  • hideWidgetOverlay

How about simplifying them into a single key, isNonInteractiveMap. When true, then the UI will hide all chrome, disable tooltips, and disable map interactions. How does that sound? So keep redux actions and selectors, but trigger from single embeddable input.

@thomasneirynck This PR hides attribution. Is that a good idea? I think attribution should never be allowed to be hidden. For uptime, none of the sources have attribution so it should not matter but I think there are legal requirements to show attribution for EMS layers using OSM data.

@nreese
Copy link
Contributor

nreese commented Nov 14, 2019

The more I think about, maybe the redux state should be simplified as well and only have a single boolean isNonInteractiveMap. GisMap should get its state from redux connectors since its a connected component instead of passing state into the component with props. addFilters and renderTooltipContent are passed in as props because there are functions and can not be stored in redux state.

@shahzad31 shahzad31 self-assigned this Nov 15, 2019
@shahzad31
Copy link
Contributor Author

@nreese i agree about simplifying redux state, though about first point, or accumulating all booleans into single booleans, i also thought of it.

Why i opted for keeping all options separate to keep it more flexible, if different needs arises, i mean if someone wants to keep some toolbar or widget , or zooming. They can individually disable/enable those.

Combining them will keep code clean, but it won't be as flexible. Maybe we can have a one redux action to pass whole object, something like
mapOptions = {zoom: false, tooltip:false, etc }
but that also doesn't look clean.

Though just having isNonInteractiveMap does looks way more clean.

WDYT?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@justinkambic
Copy link
Contributor

Why i opted for keeping all options separate to keep it more flexible

FWIW I'm +1 for flexibility too, combining the boolean flags seems like something a future consumer could run up against and want to change: "I want to enable tooltips but disable zooming".

@bcamper
Copy link

bcamper commented Nov 18, 2019

I agree having some level of granularity over controls (zoom, pan, tooltips, legend etc.) makes sense.

As for attribution: generally, it's going to be required that we show it somewhere, but it does not necessarily need to be on the map itself, e.g. it could be nearby on the page, or sometimes apps that have other non-map components will group all attribution together in a single "Data sources" link. But, there's nuance there and concern that a client from another group wouldn't know the requirements, so we may want to be more conservative and not allow it to be disabled for now, wait and see if it comes up in practice.

@justinkambic
Copy link
Contributor

we may want to be more conservative and not allow it to be disabled for now, wait and see if it comes up in practice.

👍

@shahzad31
Copy link
Contributor Author

@thomasneirynck This PR hides attribution. Is that a good idea? I think attribution should never be allowed to be hidden. For uptime, none of the sources have attribution so it should not matter but I think there are legal requirements to show attribution for EMS layers using OSM data.

@nreese @bcamper i don't understand the attribution part, i think

attributionControl: false,

was already there, i haven't changed that.

@nreese
Copy link
Contributor

nreese commented Nov 19, 2019

i don't understand the attribution part

This line in {!this.props.hideWidgetOverlay && <WidgetOverlay />} gis_map/view.js

hideWidgetOverlay should not hide the entire WidgetOverlay, just LayerControl and ViewControl portions but AttributionControl should always be visible and not be allowed to be turned off.

@shahzad31
Copy link
Contributor Author

shahzad31 commented Nov 20, 2019

@nreese i have removed the option to hide attribution, i think we can keep widgetOverlay, it shouldn't be a problem. I have also refactored redux state and have moved all props to redux.

Have also consolidated zooming control and options into interactive bool.

@elasticmachine
Copy link
Contributor

💔 Build Failed

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.

Thanks for all of the changes. This is looking really nice.

Could you please add the new map embeddable input parameters to https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/maps/public/embeddable/README.md

@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.

Thanks for making these changes.

LGTM with green CI

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor

nreese commented Nov 21, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@shahzad31 shahzad31 merged commit bc3d573 into elastic:master Nov 21, 2019
@shahzad31 shahzad31 deleted the feature/issue-49871--added-options-in-embeddable-maps branch November 21, 2019 19:13
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Nov 27, 2019
…s in embeddable maps (elastic#50663)

* added options to disable zoom, hide tool tips, widgets/overlays in embeddable maps

* revert panel changes

* added disable interactive

* remove redundant code

* update redux state and removed widget over lay hiding

* update readme with added map props
shahzad31 added a commit that referenced this pull request Nov 27, 2019
…s in embeddable maps (#50663) (#51811)

* added options to disable zoom, hide tool tips, widgets/overlays in embeddable maps

* revert panel changes

* added disable interactive

* remove redundant code

* update redux state and removed widget over lay hiding

* update readme with added map props
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 27, 2019
* upstream/7.x:
  Fix infinite redirect loop when multiple cookies are sent (elastic#50452) (elastic#51821)
  [Console] Proxy fallback (elastic#50185) (elastic#51814)
  Added endgame-* index and new heading 3 Elastic Endpoint SMP. (elastic#51071) (elastic#51828)
  [Maps] Added options to disable zoom, hide tool tips, widgets/overlays in embeddable maps (elastic#50663) (elastic#51811)
  Move errors and validate index pattern ⇒ NP (elastic#51805) (elastic#51831)
  [SIEM][Detection Engine] Adds ecs threat properties to rules (elastic#51782) (elastic#51827)
  [Lens] Remove client-side reference to server source code (elastic#51763) (elastic#51825)
  fixes drag and drop in tests (elastic#51806) (elastic#51813)
  [Uptime] Redesign/44541  new monitor list expanded row (elastic#46567) (elastic#51809)
  [7.x] [Telemetry] collector set to np (elastic#51618) (elastic#51787)
  [Uptime] added test for chart wrapper (elastic#50399) (elastic#51808)
  Expressions service fixes: better error and loading states handling (elastic#51183) (elastic#51800)
  Query String(Bar) Input - cleanup (elastic#51598) (elastic#51804)
  [ML] Adjust and re-enable categorization advanced wizard test (elastic#51005) (elastic#51017)
  fixes url state tests (elastic#51746) (elastic#51798)
  fixes browser field tests (elastic#51738) (elastic#51799)
  [Task Manager] Tests for the ability to run tasks of varying durations in parallel (elastic#51572) (elastic#51701)
  [ML] Fix anomaly detection test suite (elastic#51712) (elastic#51795)
  [SIEM] Fix Timeline drag and drop behavior (elastic#51558) (elastic#51793)
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 release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] Add ability to disable zooming for embeddable Maps
5 participants