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

Add icons for man_made=storage_tank, man_made=silo #3384

Merged
merged 3 commits into from Sep 15, 2018
Merged

Add icons for man_made=storage_tank, man_made=silo #3384

merged 3 commits into from Sep 15, 2018

Conversation

Adamant36
Copy link
Contributor

@kocio-pl
Copy link
Collaborator

Icon shape looks OK for me, but I'm not sure that z17+ is not too early for such items.

There are a lot of such objects in OSM database:
taghistory 43

@Adamant36
Copy link
Contributor Author

I was actually thinking that. What zoom level do you think would be good instead?

@kocio-pl
Copy link
Collaborator

I have already proposed z18+ - see #2909 (comment). It should be safer, but maybe someone wants to look closer at this problem.

@Adamant36
Copy link
Contributor Author

Oh, my bad. I didn't see your comment. Z18+ is the obvious zoom level though. I updated it.

@Tomasz-W Tomasz-W mentioned this pull request Sep 10, 2018
26 tasks
@kocio-pl
Copy link
Collaborator

There are some conflicts now after merging #3376, please resolve them.

@Adamant36
Copy link
Contributor Author

@kocio-pl, I resolved the conflicts, but then Travis failed. Anyway to figure out why? It seemed like everything was fine in the code.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Sep 14, 2018

Travis is right 😄 :

amenity-points.mss:3040:0 missing closing }``

When you look at this part:

    [feature = 'man_made_chimney'] { 
    [feature = 'man_made_water_works'],
    [feature = 'man_made_wastewater_plant'] { 

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.

@Adamant36
Copy link
Contributor Author

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.

@kocio-pl
Copy link
Collaborator

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.

@Adamant36
Copy link
Contributor Author

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.

@kocio-pl
Copy link
Collaborator

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.

@Adamant36
Copy link
Contributor Author

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.

@kocio-pl
Copy link
Collaborator

You're doing quite well in my opinion - thanks for your work!

@Adamant36
Copy link
Contributor Author

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.

@kocio-pl kocio-pl merged commit a3a9e59 into gravitystorm:master Sep 15, 2018
@kocio-pl
Copy link
Collaborator

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.

@Adamant36 Adamant36 deleted the storage branch September 16, 2018 03:10
@Adamant36
Copy link
Contributor Author

@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.

@kocio-pl
Copy link
Collaborator

It's from this service:

http://taghistory.raifer.tech

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

man_made=silo is not being rendered Suggestion for man_made=storage_tank
3 participants