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

Separate definitions for transportation and accommodation colors #2765

Merged
merged 2 commits into from
Aug 23, 2017

Conversation

giggls
Copy link

@giggls giggls commented Aug 21, 2017

Hello,

I was wondering if you would take such a patch.

While it would currently not change anything in the rendering output it would allow for different colors in rendering of accommodation and transportation objects, which is what I intend to do in German-style fork.

Here is an example:
acco1acco2

@giggls giggls changed the title Separate definitions for transport and accommodation colors Separate definitions for transportation and accommodation colors Aug 21, 2017
@kocio-pl
Copy link
Collaborator

It makes variable naming sane and also helps with adaptability, so I would approve it in general. However embassy is not about accomodation (and only to some degree about transportation), so I don't know how to deal with it. Any ideas?

@giggls
Copy link
Author

giggls commented Aug 21, 2017

Hm I was thinking about changing "embassy" to amenity-brown for a moment.
Probably I should have named this "touristic" instead of "accommodation"
because the same problem is with lighthouse which is also neither of both.
Embassy went in by mistake anyway thus I changed this back to
transportation in the pull request.

@giggls
Copy link
Author

giggls commented Aug 21, 2017

I found another inconsistency. I think rendering of caravan_site labels should be moved to accommodation. The current green rendering looks somewhat stange to me. I did not change the pull request yet because this would change the behavior of the renderer.
http://www.openstreetmap.org/#map=19/49.04324/8.46343

@kocio-pl
Copy link
Collaborator

I think you're right. Could you post an image how would it look like?

@giggls
Copy link
Author

giggls commented Aug 21, 2017

OK, pull request updated with a second commit.
179955caravan

@kocio-pl
Copy link
Collaborator

Makes more sense for me.

@matthijsmelissen
Copy link
Collaborator

Black is already used for man-made features/landmarks. Also blue is a quite common color for hotel icons.

Can we not change the public transport icon color - perhaps to a darker shade of blue, like tram stops?

@giggls
Copy link
Author

giggls commented Aug 22, 2017

This pull request does not change the "accommodation" rendering color on purpose.
This is just the "infrastructure" to make it easy to change, e.g. in private installations or forks like German style.

However, I do not like the blue transportation color in those objects either. I will most likely change them to brown in German style. Google Maps also uses brown for hotels.

@matthijsmelissen
Copy link
Collaborator

matthijsmelissen commented Aug 22, 2017

Ah I see, good initiative then.

It's just not clear to me though how come the icons are black in the preview?

@giggls
Copy link
Author

giggls commented Aug 22, 2017

Icons are how it would look like with @accommodation-icon and @accommodation-text set to black instead of @transportation-icon and @transportation-text.

@kocio-pl kocio-pl merged commit f707511 into gravitystorm:master Aug 23, 2017
@matthijsmelissen
Copy link
Collaborator

I didn't notice the second commit, and I would have liked to comment on it. We should make sure that PR titles match the content of the PR. And perhaps 2 days is too short for discussion. On the other hand, no harm is done by merging it, since if we change opinion we can easily revert.

The reason I would have liked to discuss this PR, is that this is basically taking a stance on #1355, a long-running discussion. In particular, by rendering the label like point, the label does no longer indicate the size of the camp ground, and it is no longer obvious that the label belongs to the green background.

Thanks for proposing this anyway @giggls!

@kocio-pl
Copy link
Collaborator

Sorry, I was sure that you've noticed both changes and forgot about this issue after 2 years (too many such long unresolved big discussions...). Probably I should have requested splitting these issues anyway. Revert should be easy in this case.

@matthijsmelissen
Copy link
Collaborator

As far as im concerned no need to revert (at least not until we decide on #1355).

@kocio-pl
Copy link
Collaborator

Hm I was thinking about changing "embassy" to amenity-brown for a moment.

I think it makes sense, since it's rather general amenity/office than transportation.

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

Successfully merging this pull request may close these issues.

None yet

3 participants