owncloud-archive / maps

:globe_with_meridians: Maps app for ownCloud
GNU Affero General Public License v3.0
42 stars 20 forks source link

Maki icons #112

Closed v1r0x closed 8 years ago

v1r0x commented 8 years ago

This PR is aimed to fix both #13 and #52.

The maki icons are now included as submodule and thus not loaded externally. Also the pink marker has been removed and the icons are displayed without any marker. What keeps this PR from merging (beside testing and review of course ;)) is a list of useful POIs and then display them depending on a certain zoom level. This should also replace the current "Places" behaviour in the nav bar. Instead giving the user a list with POIs (which will be done depending on the zoom level) to show the user can disable/enable all POIs at once.

Comments, criticism and opinions are more than welcome :) cc @jancborchardt @Henni @DJaeger

Henni commented 8 years ago

I'm not sure if git submodule is the best solution here. I currently have two problems with this approach:

  1. I haven't been able to get the icons. Currently my img/maki directory is completely empty.
  2. It looks like there is a large overhead in the maki repository. We will probably only use a small portion of it.

Maybe npm or bower would be better.

v1r0x commented 8 years ago
  1. It looks like there is a large overhead in the maki repository. We will probably only use a small portion of it.

Good point. I've never really used npm or bower. Do you have any experience and could replace the submodule approach?

v1r0x commented 8 years ago

Current version, fyi :) Hope you like the new popup style, @jancborchardt

maki-popup

Henni commented 8 years ago

@v1r0x do you use the PNGs or SVGs of maki? or in other words: which directories of maki do you need?

v1r0x commented 8 years ago

Currently PNGs, but we could switch to SVGs if we integrate them using npm or bower

Henni commented 8 years ago

118 includes the png icons via bower. Please review

jancborchardt commented 8 years ago

Let’s directly use SVG, as we do not need to support any browser which doesn’t support SVG.

Nice on the popup style and icons! Are the icons also available with a slight white border so they differentiate better from the map and when they overlay?

v1r0x commented 8 years ago

Nice on the popup style and icons! Are the icons also available with a slight white border so they differentiate better from the map and when they overlay?

I think we have to do this by ourselves. Is it possible to add a CSS border to SVG images?

jancborchardt commented 8 years ago

We can add a simple thing via CSS, for example:

.maki-icon (or whatever the class is) {
  background: #fff;
  border: 1px solid #fff;
  border-radius: 3px;
}

Of course it would be more elegant to have a true border around the icon form itself, but not sure if that’s easily possible via SVG. And we’re sure as hell not going to touch all the Maki icons. ;D

v1r0x commented 8 years ago

If it doesn't look ugly I think this would be the best solution. Or if you are bored you can edit all icons :D

v1r0x commented 8 years ago

I already removed the maki submodule. Best would be to merge master into this branch and add the border mentioned by @jancborchardt , right?

Henni commented 8 years ago

Instead of merging master you could/should rebase this branch on top of master. This keeps the history clean. Feel free to ask me if you need help with this.

v1r0x commented 8 years ago

Added a class for the marker icons. What do you think? @jancborchardt @Henni maki-icons

v1r0x commented 8 years ago

Is there anythink left in this PR? Could you please review it? Any thoughts about the icon background? @jancborchardt @Henni

Henni commented 8 years ago

Large amount of console output: image The first error might be related to https://github.com/owncloud/maps/pull/111#issuecomment-179277907

v1r0x commented 8 years ago

@Henni the Uncaught TypeError errors should be fixed. Any idea how to throttle/queue the API calls? Currently I iterate over my array using an $.each loop.

Henni commented 8 years ago

The api calls aren't part of this PR, are they? So maybe we should fix this in a separate PR.

v1r0x commented 8 years ago

I replaced the whole POI stuff in this PR, so basically it is a part of this PR. But we also have to include all missing POI types. So we could close this PR and only fix #52 and fix #13 including the api calls in another PR.

Henni commented 8 years ago

Let's do this then! One last squash and this should be ready to merge

v1r0x commented 8 years ago

Ready to merge?