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] Map settings: min and max zoom #63714

Merged
merged 20 commits into from
Apr 21, 2020
Merged

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Apr 16, 2020

This PR lays the foundation for Map properties as described in #62769. This PR implements min and max zoom map properties.

Screen Shot 2020-04-20 at 1 22 40 PM

Screen Shot 2020-04-16 at 8 30 45 AM

@nreese nreese requested review from a team as code owners April 16, 2020 14:35
@nreese nreese added release_note:enhancement [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.8.0 v8.0.0 labels Apr 16, 2020
@elasticmachine
Copy link
Contributor

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

@nreese
Copy link
Contributor Author

nreese commented Apr 16, 2020

@elastic/kibana-design

The open button is currently a gear icon in the Legend. This is just a place holder until a better location can be found. Please advise.

Screen Shot 2020-04-16 at 8 34 26 AM

@cchaos
Copy link
Contributor

cchaos commented Apr 16, 2020

Since this isn't something that should be manipulated in the view/embedded state, I'd assume put the options in the top menu. If there's only a few, you can keep them in a popover. If there will be a long list, use a normal Flyout. Not one that pushes the maps like the edit layer.

@nreese nreese changed the title Map settings [Maps] Map settings: min and max zoom Apr 16, 2020
@nreese
Copy link
Contributor Author

nreese commented Apr 16, 2020

Since this isn't something that should be manipulated in the view/embedded state, I'd assume put the options in the top menu.

If could be in the embedded state in the future. There is talk of making Dashboard the home for everything and we want all edit UIs to be part of embeddable.

If there's only a few, you can keep them in a popover.

The list will get rather large as all properties are implemented. Why would you use a

If there will be a long list, use a normal Flyout. Not one that pushes the maps like the edit layer.

Why would you not use the same flyout as the edit layer? Users will still want to see the entire map to understand how the settings effect the map.

@cchaos
Copy link
Contributor

cchaos commented Apr 16, 2020

If could be in the embedded state in the future. There is talk of making Dashboard the home for everything and we want all edit UIs to be part of embeddable.

Then it would be handled like as part of the dashboard panel and not internal to the embedded display via the panel options menu (and then only available in edit mode).

Screen Shot 2020-04-16 at 11 57 44 AM

If there will be a long list, use a normal Flyout. Not one that pushes the maps like the edit layer.

This is the same way we handle the settings for Graph, Dev tools, etc... If it is whole object-wide settings, it is a menu item. Using a regular flyout actually allows them to focus on the changes that effect the map and ignore the layers panel.

Image 2020-04-16 at 11 58 44 AM

@nreese
Copy link
Contributor Author

nreese commented Apr 20, 2020

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@nreese
Copy link
Contributor Author

nreese commented Apr 20, 2020

@cchaos I have moved the action button open the menu to the top level chrome. I would still like to use the flyout that pushes the map over for 2 reasons

  1. consistency in the UI for the flyout panel.
  2. To avoid covering the map legend. Settings will effect layers and those effects will be surfaced in the map legend. Users need to see those effects while setting values.

Screen Shot 2020-04-20 at 1 22 40 PM

@cchaos cchaos requested a review from miukimiu April 21, 2020 14:38
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

Will be nice UI to collect map-properties!

I think with these new zoom restriction, it will help if we make the parts related to zoom in the UX more consistent:

  1. Is is possible to make the zoom-readout behave accordingly with the slider when the map snaps lower or higher?

image

It really only updates on move-changes, so when the map snaps to a lower/higher zoom, this is not being indicated on the map. (? The moveend event here might not be the one we want:

this.props.extentChanged(this._getMapState());
)

  1. Possible to validate the zoom-input to the allowed range?

image

  1. (less sure about this one, maybe more of a nice2have). Since we have the immediate visual feedback with the zoom-snapping and then also the rollback behavior, if users cancel out the zoom-range settings, shouldn't the map snap back to the old setting (similar to e.g. color changes)? (this will prevent users from playing around and losing their position)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

thx!

@@ -591,6 +608,22 @@ app.controller(
getInspector().open(inspectorAdapters, {});
},
},
{
id: 'mapSettings',
label: i18n.translate('xpack.maps.mapController.openSettingsButtonLabel', {
Copy link
Contributor

Choose a reason for hiding this comment

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

good call moving this to left of save

@nreese nreese merged commit ea4eb33 into elastic:master Apr 21, 2020
nreese added a commit to nreese/kibana that referenced this pull request Apr 21, 2020
* [Maps] Map settings: min and max zoom

* eslint

* header and footer

* zoom range UI

* save session state in mapStateJSON

* disable button when flyout is open

* tslint

* update layer_control jest test

* tslint

* move settings button to top map chrome

* move map_settings_panel to NP

* remove merge conflict artifact

* fix import for NP migration

* remove unused CSS class

* fix path from NP move

* review feedback

* load map settings in embeddable
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 22, 2020
* master: (29 commits)
  [Dashboard] Deangularize navbar, attempt nr. 2 (elastic#61611)
  refactor action filter creation utils (elastic#62969)
  Refresh index pattern list before redirecting (elastic#63329)
  [APM]fixing custom link unit tests (elastic#64045)
  [Ingest] EPM & Fleet are enabled when Ingest is enabled (elastic#64103)
  [Alerting] Fixed bug with no possibility to edit the index name after adding (elastic#64033)
  [Maps] Map settings: min and max zoom (elastic#63714)
  [kbn-storybook] Use raw loader for text files (elastic#64108)
  [EPM] /packages/{package} endpoint to support upgrades (elastic#63629)
  [SIEM] New Platform Saved Objects Registration (elastic#64029)
  [Endpoint] Hook to handle events needing navigation via Router (elastic#63863)
  Fixed small issue in clone functionality (elastic#64085)
  [Endpoint]EMT-146: use ingest agent for status info (elastic#63921)
  [SIEM] Server NP Followup (elastic#64010)
  Register uiSettings on New Platform (elastic#64015)
  [Reporting] Integration polling config with client code (elastic#63754)
  [Docs]7.7 SIEM doc updates (elastic#63951)
  [SIEM] [Cases] Tags suggestions (elastic#63878)
  Include datasource UUID in agent config yaml, adjust overflow height of yaml view (elastic#64027)
  [DOCS] Add file size setting for Data Visualizer (elastic#64006)
  ...
nreese added a commit that referenced this pull request Apr 22, 2020
* [Maps] Map settings: min and max zoom

* eslint

* header and footer

* zoom range UI

* save session state in mapStateJSON

* disable button when flyout is open

* tslint

* update layer_control jest test

* tslint

* move settings button to top map chrome

* move map_settings_panel to NP

* remove merge conflict artifact

* fix import for NP migration

* remove unused CSS class

* fix path from NP move

* review feedback

* load map settings in embeddable
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 22, 2020
…ana into task-manager/cancel-logging

* 'task-manager/cancel-logging' of github.com:gmmorris/kibana: (28 commits)
  [Dashboard] Deangularize navbar, attempt nr. 2 (elastic#61611)
  refactor action filter creation utils (elastic#62969)
  Refresh index pattern list before redirecting (elastic#63329)
  [APM]fixing custom link unit tests (elastic#64045)
  [Ingest] EPM & Fleet are enabled when Ingest is enabled (elastic#64103)
  [Alerting] Fixed bug with no possibility to edit the index name after adding (elastic#64033)
  [Maps] Map settings: min and max zoom (elastic#63714)
  [kbn-storybook] Use raw loader for text files (elastic#64108)
  [EPM] /packages/{package} endpoint to support upgrades (elastic#63629)
  [SIEM] New Platform Saved Objects Registration (elastic#64029)
  [Endpoint] Hook to handle events needing navigation via Router (elastic#63863)
  Fixed small issue in clone functionality (elastic#64085)
  [Endpoint]EMT-146: use ingest agent for status info (elastic#63921)
  [SIEM] Server NP Followup (elastic#64010)
  Register uiSettings on New Platform (elastic#64015)
  [Reporting] Integration polling config with client code (elastic#63754)
  [Docs]7.7 SIEM doc updates (elastic#63951)
  [SIEM] [Cases] Tags suggestions (elastic#63878)
  Include datasource UUID in agent config yaml, adjust overflow height of yaml view (elastic#64027)
  [DOCS] Add file size setting for Data Visualizer (elastic#64006)
  ...
@jsanz jsanz mentioned this pull request Apr 23, 2020
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.

5 participants