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

Tilemap zoom precision #3713

Closed
wants to merge 12 commits into from

Conversation

jthomassie
Copy link

Added option to change precision as you zoom in on a tile map
Closes #2618, closes #2034.
• added param and input checkbox to agg editor, defaults to true
• hides precision slider if true
• added mapZoomEnd event to dispatch

@rashidkpc
Copy link
Contributor

The description says this option is enabled by default, but this doesn't seem to be the case?

Also, checked or unchecked, I'm not getting the auto-precision.

2015-04-29 12_21_29

Can you write tests for this behavior?

@rashidkpc rashidkpc assigned jthomassie and unassigned rashidkpc Apr 29, 2015
@jthomassie jthomassie assigned rashidkpc and unassigned jthomassie Apr 29, 2015
@jthomassie
Copy link
Author

@rashidkpc please take a look now. Lost the mapZoomEnd event code when I merged in master.

@@ -125,6 +125,46 @@ define(function (require) {

editableVis.listeners.click = vis.listeners.click = filterBarClickHandler($state);
editableVis.listeners.brush = vis.listeners.brush = brushEvent;
editableVis.listeners.mapZoomEnd = vis.listeners.mapZoomEnd = function (event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the idea of sticking map specific code in editor.js. Couldn't this be done in src/kibana/plugins/vis_types/vislib/tile_map.js?

You might take a look at how the rectangle event is handled for the tilemap filter here: https://github.com/elastic/kibana/pull/3671/files

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, I wIll work on this.

Copy link
Author

Choose a reason for hiding this comment

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

Oops, I meant to do that. Fixed now.

@rashidkpc rashidkpc assigned jthomassie and unassigned rashidkpc Apr 29, 2015
jthomassie pushed a commit to jthomassie/kibana that referenced this pull request Apr 30, 2015
@rashidkpc rashidkpc assigned rashidkpc and unassigned jthomassie May 4, 2015
@@ -126,6 +126,47 @@ define(function (require) {
editableVis.listeners.click = vis.listeners.click = filterBarClickHandler($state);
editableVis.listeners.brush = vis.listeners.brush = brushEvent;

// editableVis.listeners.mapZoomEnd = vis.listeners.mapZoomEnd = function (event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove all this commented out code

Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to be removed.

@rashidkpc rashidkpc assigned jthomassie and unassigned rashidkpc May 4, 2015
@jthomassie jthomassie assigned rashidkpc and unassigned jthomassie May 5, 2015
@jthomassie
Copy link
Author

@rashidkpc I've made updates to address the latest comments and merged master. Please take a look.

@rashidkpc rashidkpc assigned jthomassie and unassigned rashidkpc May 5, 2015
@@ -31,6 +32,38 @@ define(function (require) {
filter.geo_bounding_box[field] = event.bounds;

pushFilter(filter, false, indexPatternName);
},
mapZoomEnd: function (event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't check if autoPrecision is turned on, so there's no way to disable it.

Copy link
Author

Choose a reason for hiding this comment

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

Adding autoPrecision attr to the mapZoomEnd event to check for this.

@rashidkpc
Copy link
Contributor

This causes all of the maps on the page to zoom in to the same spot when you zoom one of them:

2015-05-05 09_14_35

@jthomassie
Copy link
Author

Yes, using courier.fetch to redraw the maps is causing this. The maps get destroyed and redrawn at 'zooomend' if autoPrecision attr is true.

@jthomassie
Copy link
Author

@rashidkpc please take a look at this again. I am not sure if there is a way around the split maps all redrawing on zoom if autoPrecision is true until we can persist the map states. Do you have an idea how we might get around this?

@jthomassie jthomassie assigned rashidkpc and unassigned jthomassie May 5, 2015
@rashidkpc rashidkpc assigned jthomassie and unassigned rashidkpc May 5, 2015
@rashidkpc
Copy link
Contributor

This doesn't seem to increase precision beyond 7 anymore?

@rashidkpc
Copy link
Contributor

Because the precision isn't actually bound to the zoom level, stuff like this happens, where toggling the option off then on again causes the precision to go back to 2. Not neccesarily a :

2015-05-07 13_29_57

@rashidkpc rashidkpc assigned spalger and unassigned jthomassie May 13, 2015
@spalger spalger mentioned this pull request May 13, 2015
@spalger spalger closed this May 13, 2015
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.

Adjust dot size with zoom level Tile maps - increase precision on zoom
3 participants