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 icon for bicycle repair station, update car repair icon #3391

Merged
merged 1 commit into from
Sep 21, 2018
Merged

Add icon for bicycle repair station, update car repair icon #3391

merged 1 commit into from
Sep 21, 2018

Conversation

Adamant36
Copy link
Contributor

@Adamant36 Adamant36 commented Sep 14, 2018

This PR adds an icon for bicycle repair station and updates the car repair icon, fixes #3069. As bicycle repair stations are not suppose to mapped as ways and therefore there was not any mapped that way within the area tested, I did include a test rendering example of it.
https://www.openstreetmap.org/#map=19/37.83791/-122.29701 (node)
bicycle repair station node

new car repair icon
https://www.openstreetmap.org/#map=19/37.87100/-122.27196 (way)
car repair way
https://www.openstreetmap.org/#map=19/37.87298/-122.27321 (node)
car repair node

@Tomasz-W Tomasz-W mentioned this pull request Sep 14, 2018
26 tasks
@polarbearing
Copy link
Contributor

polarbearing commented Sep 14, 2018

On the third example, I see a purple car repair with brown label? probably #2658

For the bike repair station, wasn't there a discussion to distinguish self-repair stations from repair-service, by colour?

@Adamant36
Copy link
Contributor Author

Adamant36 commented Sep 14, 2018

@polarbearing, yeah its related to the issue you referenced. I think I figured out whats causing it in the code, but I rather deal with it in that issue. As far as the color goes, I wasn't involved in the original discussion, but just from a glance over it you had brought it up a few times and other people said not to get hung up on the color thing. Then me and few other people, who I think where involved in it, decided this was the color to go with for this.

If you want to summarize your original points about the color here, plus the pros and cons, go for it. It seems like it was already decided though. Although as I said, I wasn't involved in the original discussion. So I might be wrong. Personally, I think this PR is just about getting bicycle repair stations rendered in the first place. Plus, it is an amenity. I don't see why the whole self-repair station versus repair-service thing can't be another, separate issue after it is at least rendered if need be. Since its clearly a separate issue then it being rendered in the first place.

Aren't all bicycle repair stations self serving and therefore amenities?

@polarbearing
Copy link
Contributor

My point is to distinguish self-repair stations from repair-service. Thus we can go ahead with amenity-brown for the self-service here, and should reconsider the colour for the attended car-repair, which can be done in #3395.

@Adamant36
Copy link
Contributor Author

OK. I agree with you that car-repair color should be reconsidered. The new issue about colors in general is a good one also. I think its worth more discussion and id be happy to do the coding/PRs for whatever is decided there if need be.

@Adamant36 Adamant36 changed the title Add icon for bicycle repair station, updates car repair icon Add icon for bicycle repair station, update car repair icon Sep 15, 2018
@Tomasz-W
Copy link

I think in both cases we should use brown:

  • There is no tag distinguishing self-repair bicycle stations from repair services ones. Please see https://wiki.openstreetmap.org/wiki/Tag:amenity=bicycle_repair_station , by the Wiki, repair services should be tagged as a shop=bicycle, and this tag surely should be rendered in shop-violet
  • As car repair stations are usually a place where you are not paying for some good, but you are lefting you car, and some time later pay for the service, I think they shouldn't be shop-violet but amenity-brown

@dieterdreist
Copy link

dieterdreist commented Sep 15, 2018 via email

@Tomasz-W
Copy link

Tomasz-W commented Sep 15, 2018

places where you can go to fix your car yourself, paying a fee for utilizing tools and workshop

Well, I didn't hear about places like that, because in Poland it's always a place where you give your car to the mechanic to take care of. I think that shops focused on selling services may avoid shop-violet, because it can be confused with eg. shop with car tools in this case, and amenity-brown suggest kind of service there. Anyway, even if there are tool rental ones, they are also still more like a service thing than a shop.

@dieterdreist
Copy link

dieterdreist commented Sep 15, 2018 via email

@polarbearing
Copy link
Contributor

May I conclude that keeping the bike-self-repair brown in this PR is not doing any harm, so let us merge this, and continue the service vs self-service thing in #3395, or a new issue.

@kocio-pl
Copy link
Collaborator

Adding icon for bicycle repair station is quite clear, but I'm not sure what's different with car repair icon here? Is it just pixel aligned somehow or what?

It's not good to mix different things in one PR, because in general they could be accepted or rejected separately.

@Adamant36
Copy link
Contributor Author

It makes the wrench in the car repair icon look the same as the one for bicycle repair stations. With the handle facing toward the same direction and the wrench being thinner. I should have specified that in the original PR. I also agree that changes to multiple things should be in different PRs. I've been trying to steer people towards doing that more when multiple changes come up in the same issue, but I'll do more on my end with the actual PRs to make sure things are done separate when they should be.

@kocio-pl
Copy link
Collaborator

OK, I see now. I will test it soon.

I should have specified that in the original PR.

That would help me a lot. That's why I've made a PR template with dull items to fill. 😄

@kocio-pl
Copy link
Collaborator

Such stations are quite small, as seen on the wiki page and they belong to z19+. They are smaller than some of the bicycle parkings or bicycle rental stations and I have seen them as a supporting amenity for them.

@Adamant36
Copy link
Contributor Author

OK. I updated it to z19.

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

I checked that it works also for named ways (this one should be bicycle shop probably).

Thanks for the code and being so responsive!

@Adamant36
Copy link
Contributor Author

Your welcome. Thanks for double checking the named ways and for getting it merged in time for the update. It was a really good cordination job with everyone. I bet Tomasz-W will be happy to see it on the map before winter 😄

@Adamant36 Adamant36 deleted the bicycle branch September 23, 2018 04:01
@jeisenbe jeisenbe added new features Requests to render new features POI labels Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new features Requests to render new features POI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rendering for amenity=bicycle_repair_station
6 participants