Skip to content
This repository was archived by the owner on Nov 2, 2018. It is now read-only.

Conversation

@v1r0x
Copy link
Contributor

@v1r0x v1r0x commented Jan 9, 2016

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
Copy link
Contributor

Henni commented Jan 28, 2016

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
Copy link
Contributor Author

v1r0x commented Jan 28, 2016

  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
Copy link
Contributor Author

v1r0x commented Jan 28, 2016

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

maki-popup

@Henni
Copy link
Contributor

Henni commented Jan 28, 2016

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

@v1r0x
Copy link
Contributor Author

v1r0x commented Jan 28, 2016

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

@Henni
Copy link
Contributor

Henni commented Jan 28, 2016

#118 includes the png icons via bower. Please review

@jancborchardt
Copy link
Contributor

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
Copy link
Contributor Author

v1r0x commented Jan 28, 2016

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
Copy link
Contributor

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
Copy link
Contributor Author

v1r0x commented Jan 28, 2016

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
Copy link
Contributor Author

v1r0x commented Jan 29, 2016

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

@Henni
Copy link
Contributor

Henni commented Jan 29, 2016

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
Copy link
Contributor Author

v1r0x commented Jan 29, 2016

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

@v1r0x
Copy link
Contributor Author

v1r0x commented Feb 3, 2016

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

@Henni
Copy link
Contributor

Henni commented Feb 3, 2016

Large amount of console output:
image
The first error might be related to #111 (comment)

@v1r0x
Copy link
Contributor Author

v1r0x commented Feb 3, 2016

@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
Copy link
Contributor

Henni commented Feb 3, 2016

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

@v1r0x
Copy link
Contributor Author

v1r0x commented Feb 3, 2016

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
Copy link
Contributor

Henni commented Feb 3, 2016

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

@v1r0x
Copy link
Contributor Author

v1r0x commented Feb 3, 2016

Ready to merge?

Henni added a commit that referenced this pull request Feb 3, 2016
@Henni Henni merged commit 5cb984c into master Feb 3, 2016
@Henni Henni deleted the maki-icons branch February 3, 2016 23:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants