-
Notifications
You must be signed in to change notification settings - Fork 819
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
Add icons for man_made=storage_tank, man_made=silo #3384
Conversation
I was actually thinking that. What zoom level do you think would be good instead? |
I have already proposed z18+ - see #2909 (comment). It should be safer, but maybe someone wants to look closer at this problem. |
Oh, my bad. I didn't see your comment. Z18+ is the obvious zoom level though. I updated it. |
There are some conflicts now after merging #3376, please resolve them. |
@kocio-pl, I resolved the conflicts, but then Travis failed. Anyway to figure out why? It seemed like everything was fine in the code. |
Travis is right 😄 :
When you look at this part:
you will notice that after chimney line there is a bracket instead of comma. This is why I insist on checking every change with rendering test. You would know there's a problem if you would fire a Kosmtik just in case. 😄 This kind of errors is very common and simple rendering test is enough to catch them. |
Ah ha. Good spot. Travis sure knows what he/she is doing. Unfortunately, I resolved the conflicts in the website editor this time. So there was no way to run Kosmtik to test it this time. I usually do though. |
You can simply pull the changes to your local repository (as I did) and then test it. I advise to always check the code changes if possible. |
Hhhhmmm, for some reason I assumed that if I did that it wouldn't pull the merge conflict with it for some reason. Since it would be pulling from the branch or something and wouldn't fast forward whatever changes there were in meantime. Or it would just pull from the upstream repository and screw everything up somehow. I'll have to try that next time though. Its a lot to keep track of. |
If you have local branch for that, you can always try to pull. And if you're not feeling safe, you can make another branch of this branch just to test it. However I understand the difficulty of playing with different problems when you just want to update the code. Now I understand git better in the scope of this repo, but it took me some time to learn it. |
OK. Good to know there is that option. It can definitely feel like I'm in way over my head sometimes. Especially with a bunch of different branches and PRs going at once. At least it was a fairly simple problem this time though. |
You're doing quite well in my opinion - thanks for your work! |
Thanks. I apreciate that. Its been fun to learn everything and over all I really enjoy contributing to the project. Even when it doesnt go so well 😄 It really helps that both you and Tomasz-W have been extremely helpful when I needed it. |
Thanks, this is working good. z18+ looks like good initial level since some of them can be quite small, another thing for the future might be color tuning, since they are quite heavy/dark too. |
@kocio-pl, your welcome. Hey, whats the website for showing the chart of tag usage like you have in the message above? I can't seem to find a link to it anywhere. |
It's from this service: |
This PR adds icons for both man_made=storage_tank and man_made=silo. Closes #588, closes #2909
storage tank
https://www.openstreetmap.org/#map=19/37.97589/-122.66540 (node)
https://www.openstreetmap.org/#map=19/37.94376/-122.51119 (way)
Silo
https://www.openstreetmap.org/#map=19/38.17840/-122.81463 (node)
https://www.openstreetmap.org/#map=19/38.21552/-122.60608 (way)