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

Implement shared zoom state #6914

Closed
wants to merge 5 commits into from

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Apr 14, 2016

This change implements a way to save the zoom and center position of the map using uiState.

It does this by doing two things:

  • give aggConfig objects uiState. They don't have to use it, and it's optional so they have to be written to function without it, but when it's available it aggregations can use it to be effected by a visualization. This most commonly means that the aggregation can change "auto" sizing behavior based on different ui properties like the size of the visualization or in the case of the map it's zoom level
  • writes and reads the mapZoom and mapCenter to the aggregation's uiState from the visualization.

spalger added 5 commits April 12, 2016 16:07
Until we have a way to only listen for changes from outside of the visualization, or a way to tell the visualization not to render for specific changes, this is what we have to do
@rashidkpc
Copy link
Contributor

@w33ble please have @panda01 take a look at this as well

@rashidkpc rashidkpc mentioned this pull request Apr 14, 2016
@panda01
Copy link
Contributor

panda01 commented Apr 15, 2016

@spalger showed me this pr as some help with my implementation of the same thing. I'm not a fan of this, because i think it does too much. I don't think aggConfigs need a uiState, when they all have access to the vis which already has one.

@spalger
Copy link
Contributor Author

spalger commented Apr 15, 2016

One of the goals with the uiState design was to support sub-states, which is what the states attached to each aggConfig are, a sub-state of the visualization uiState.

The reason I prefer the sub-state implementation is because it links the state values for an agg to that agg; if that agg gets deleted so does it's UI state, multiple aggs can have separate uiStates, etc.

if some state value makes more sense at the vis level it can still go there, but I think that having agg-level uiStates is a win overall.

@jazzzz
Copy link

jazzzz commented Apr 15, 2016

Wouldn't it be better to save the map bounds instead of the zoom and center? When I move the map to fit a country, I want the map to fit this country in the dashboard with a smaller map, and I think you cannot do this with only the zoom and center.

@panda01
Copy link
Contributor

panda01 commented Apr 15, 2016

Hmm, idk how i feel about that, @spalger, this is the only scenario we would have to have developed it for so far, and why build something extra, and add more abstraction for one thing? Also they already have params which I'm investigating currently but seem to be persisted someway, probably after $state.save()

I can see kind of where you're coming from I just don't think the Agg is the correct place for this, Perhaps we can move it to the vis, it's already there, just kind of not really implemented, this can also be linked to a child state based off of the ID but will enable us to do a whole bunch of cool things on the dashboard like panel specific filters.

To sum up; the AggConfig.uiState is too specific a scenario IMO, and I can't see us using it too much in the future, that vis though...

@panda01
Copy link
Contributor

panda01 commented Apr 15, 2016

@jazzzz the zoom and the center accomplishes everything you need, pull this or #6835 to give it a try.

@jazzzz
Copy link

jazzzz commented Apr 15, 2016

@jazzzz the zoom and the center accomplishes everything you need, pull this or #6835 to give it a try.

OK. Just curious, are the width and height also saved to accomplish this?

@spalger
Copy link
Contributor Author

spalger commented Apr 15, 2016

@jazzzz we currently pick the geohash precision based on the zoom level in a static map. The center point is just there to restore the focus of the visualization.

@panda01 it's your change, so I'll let you implement it how you like. I agree now that aggregation-level uiState really isn't useful for two reasons:

  1. visualizations shouldn't have to know about aggregations (in this pull the tilemap has to look for and interact with the geohash agg directly)
  2. I can only think of useful uiState values that are vis or bucket level, nothing aggregation level.
    • Even the uiState values from this pull should really be stored on at the vis level, they are global to the visualization
    • other uiState <-> Vis ideas:
      • based on size of visualization size:
        • auto interval for date histogram
        • auto term size
      • enable/disable aggregations based on visible items (could be agg level state, but vis shouldn't have to translate visual feature to aggregation)

@spalger spalger closed this Apr 15, 2016
@spalger spalger deleted the implement/sharedZoomState branch October 18, 2019 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants