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

Even better map #1166

Closed
wants to merge 19 commits into from
Closed

Even better map #1166

wants to merge 19 commits into from

Conversation

eriky
Copy link

@eriky eriky commented Apr 19, 2014

Upgrade to bettermap: you can now select regions on the map by drawing a polygon. The regions are linked to filters. You can remove either the filter or the region on the map, both will work.
To accomodate this feature I have also:

  • upgraded leaflet,
  • upgraded the marker cluster plugin,
  • added the leaflet.draw plugin.
  • made the OSM url configurable

Finally, I have added some code the the filterServer and to the filtering panel to accomodate geo_polygon filter types.

eriky added 2 commits April 19, 2014 21:52
…g a polygon. The regions are linked to filters. You can remove either the filter or the region on the map, both will work. To accomodate this feature I have also: upgraded leaflet, upgraded the marker cluster plugin, added the leaflet.draw plugin. I also made the OSM url configurable. Finally, I have added some code the the filterServer and to the filtering panel to accomodate geo_polygon filter types.
@mithun35h
Copy link

Looks like Great feature.

@eriky
Copy link
Author

eriky commented Apr 22, 2014

One problem I discovered today: when I use grunt build, somehow the leaflet.js file is not included in the final dist version. I'm very new to this and so far haven't found the reason. Could just as well be a problem on my local machine.

@rashidkpc
Copy link
Contributor

Thanks, will get this on the list for review. Might take a bit, there's a lot here.

@rashidkpc rashidkpc closed this in 45ff278 Apr 28, 2014
@rashidkpc rashidkpc reopened this Apr 28, 2014
@rashidkpc
Copy link
Contributor

Accidentally closed that, sorry, reopened.

@rashidkpc
Copy link
Contributor

A few notes:

  • The red boxes are not recreated on load. They should either not be shown ever, or should be recreated. I lean towards not shown ever. Eg, show them while creating the filter, then destroy.
  • The way filters are displayed there are no details for the filter, making it hard to tell which geo_polygon filter is which.
  • The edit and remove buttons are confusing, probably better to just drop them and have filters be added/removed from kibana's filtering bar
  • Drill down doesn't really work since the filters are setup as either. If you draw a box inside an existing box nothing will change.

@eriky
Copy link
Author

eriky commented Apr 28, 2014

All good points. I can think of to ways to solve these:

  1. I could only allow one box to be drawn. Once the box is drawn, it is
    hidden/deleted from the map. This solves all points as far as I can see.

  2. Alternatively I can allow one box and find a way to redraw it on reload
    of a saved dashboard. I personally prefer to keep the red box because it
    quickly visualizes the fact that a geo filter is present.

I'm not sure if editing a selection is an important enough feature to
justify the extra complexity. What do you think? Just drop the feature?

2014-04-28 22:32 GMT+02:00 Rashid Khan notifications@github.com:

A few notes:

  • The red boxes are not recreated on load. They should either not be
    shown ever, or should be recreated. I lean towards not shown ever. Eg, show
    them while creating the filter, then destroy.
  • The way filters are displayed there are no details for the filter,
    making it hard to tell which geo_polygon filter is which.
  • The edit and remove buttons are confusing, probably better to just
    drop them and have filters be added/removed from kibana's filtering bar
  • Drill down doesn't really work since the filters are setup as
    either. If you draw a box inside an existing box nothing will change.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1166#issuecomment-41609578
.

@rashidkpc
Copy link
Contributor

I'd vote to just drop the editing functionality and only allow one box to be drawn. If another box is drawn it overwrites the old one.

Keeping the red box is fine as long as it repopulates when the dashboard is loaded.

@eriky
Copy link
Author

eriky commented Apr 28, 2014

Ok so I have some work to do ;-)

2014-04-28 22:58 GMT+02:00 Rashid Khan notifications@github.com:

I'd vote to just drop the editing functionality and only allow one box to
be drawn. If another box is drawn it overwrites the old one.

Keeping the red box is fine as long as it repopulates when the dashboard
is loaded.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1166#issuecomment-41612693
.

@eriky
Copy link
Author

eriky commented Apr 29, 2014

@rashidkpc I removed the edit/delete controls. Drawing a selection while another is still there will replace it. Selections stay on the map. A geo_polygon filter now also shows the field on which the filter is active.
And, as can be seen below, I fixed a small bug which was already present in bettermap.

@rashidkpc
Copy link
Contributor

Looks better, currently a couple more issues

  1. In its current state it does not pass the build process. Please run grunt build and ensure the build completes and the code works in its built state.
  2. It might be better if the red area was shown while drawing the selection, then immediately removed, for two reasons. a) If you draw another rectangle, then remove the filter, there is no red selection shown, though a geo_polygon filter is still present and b) If you save the dashboard with a geo_polygon filter, the red area is not redrawn when you load it. Simply getting rid of the red after the filter is created each time will solve this
  3. Editing the geo_polygon filter causes the filter to fail. Disabling editing of geo_polygons on line 100 of the filtering panel's module.js would be an acceptable solution here.

@eriky
Copy link
Author

eriky commented May 7, 2014

Ok here's the current state:

  1. it passes the build process
  2. Red area is removed after making a selection. A new selection replaces the old selection.
  1. Edit of geo_polygon filter is now disabled

Just FYI: it took me a long time to fix an annoying issue where image urls in vendor css got rewritten. Eventually I managed to fix this by editing tasks/options/cssmin.js and adding a relative root url. Initially I also updated the Grunfile.js to newer versions of all the packages, but I reverted this since it was not needed. I would still recommend you to upgrade to the newer versions of all packages. If you want I can offer that file as a seperate pull request.

@mithun35h
Copy link

+1

mrdavidlaing added a commit to cityindex/kibana that referenced this pull request Jun 6, 2014
…to_v3.1.0

* commit 'c19105cf3f2dbefe7aa41caebca5bcf40efff3b7': (51 commits)
  Fixed spurious version too low message when elasticsearch is unreachable
  Update module.js
  fixed typo preventing filters from being applied to topN queries
  Removing console.log message
  Remove apply, update version to 3.1.0
  Add panel height params back in to fix column panel sizing. Closes elastic#1146
  Closes elastic#1206. Fix share url in firefox
  Fix box-sizing in firefox, fix spinner pushing down panel-extra-container, closes elastic#1218
  Fix column panel after 430d7fa.
  Add jsonpath to license file
  Reverting elastic#1163, the overhead is larger than I anticipated and regularly grabbing empty data sets causes some visual oddities
  Add margin to bottom of error
  Add ignore_unmapped to sorts, closes elastic#1153
  Simplify docs, add tooltip, clean up editor
  Use a field filter instead of terms filter so that analysis is done on the value. Closes elastic#1166. Closes elastic#1142
  Send render whenever window size changes or a panel is resized
  Simplify kibanaPanel directive, fix css that stopped overflow:scroll from working on the table panel
  Fix for possibility of partial spans
  remove commented out less
  Add drag to resize panel widths. Closes elastic#329
  ...
@robbiet480
Copy link

@eriky I was just about to go ahead and improve the map, but I see you've done a lot of the hard work for us already! I just have a few quick notes that would improve the experience more:

  • The MapQuest attribution is incorrect. It should be Tiles Courtesy of <a href="http://www.mapquest.com/" target="_blank">MapQuest</a> <img src="http://developer.mapquest.com/content/osm/mq_logo.png"> as per the terms of use (bottom of page)
  • If you change the MapQuest url to //otile{s}-s.mqcdn.com/tiles/1.0.0/map/{z}/{x}/{y}.jpg and set subdomains to '1234', tiles will load faster, and will not break SSL. otileN-s is barely documented anywhere, but we at @MapJam are using it in production just fine. It will load tiles over HTTP or HTTPS whereas without the -s the tiles will only load over HTTP.
  • For all tile layers, enabling detectRetina is a big plus for us retina screen owners
  • leaflet-providers would be a great addition. It comes with a bunch of tile providers and 0 config, so users could set what they want. I saw your custom URL support section, that is also great but you could also allow setting the default tileset, which you can pass to leaflet-providers by using their excellent pre-filled layer control

If you aren't able to do any of this, I would be more than willing to take some time to do it. The current bettermap module hurts my soul, but only because I develop full time with Leaflet and know it's intricacies and problem spots.

@rashidkpc /other kibana admins, it would be great to accept @eriky's changeset with or without my suggestions. Leaflet 0.6-dev is getting long in the tooth now, and Leaflet 0.7 has huge performance gains/better browser and mobile support.

Thanks

@eriky
Copy link
Author

eriky commented Aug 8, 2014

Hey @robbiet480, unfortunately I don't have time to add any more changes. I'd love to see your changes added so if you want to work on it: great! Maybe @rashidkpc can comment too. I'd be nice to see these improvements in Kibana at some point in the future ;-)

@robbiet480
Copy link

@eriky Great, i'll take care of them today hopefully. Thanks for the quick response.

@rashidkpc Is it a safe assumption that the changes would be accepted soon? This issue has been sitting for a while. Normally I wouldn't ask, but Leaflet 0.8 is coming out soon supposedly and it would be good to just wait for that if the issue is going to sit for a while.

@w33ble w33ble closed this Oct 6, 2014
@spalger
Copy link
Contributor

spalger commented Oct 6, 2014

Woops! Sorry about that! We recently replaced the master branch with Kibana 4. This action force closed all of the old pull requests against master. We will be reviewing these on a case-by-case basis and creating new tickets as necessary.

The good news is that many long requested features can be found in Kibana 4, and we're being entirely open about our roadmap. Check out the roadmap tickets (which we're still filling in) here.

If you're looking for the old Kibana 3 code you can find it here.

@w33ble
Copy link
Contributor

w33ble commented Oct 8, 2014

We're stopping development on Kibana 3 and focusing on Kibana 4 going forward. Since Kibana 4 won't be using Bettermaps, I'm going to leave this closed.

@LRParser
Copy link

What will Kibana4 be using in place of Bettermaps? Is any replacement planned? This was an awesome feature. Looked through the milestones but couldn't find anything relevant.

@LRParser
Copy link

Nevermind, I see this: #1549

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.

7 participants