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 shop=books #1311

Merged
merged 1 commit into from
Mar 20, 2015
Merged

add icon for shop=books #1311

merged 1 commit into from
Mar 20, 2015

Conversation

nebulon42
Copy link
Contributor

Preview:
books

@matthijsmelissen
Copy link
Collaborator

Tester recognizes icon as books.

@matkoniecz
Copy link
Contributor

Tester recognizes icon as books. Maybe it would be a good idea to have the same icon in two colours for shop=books and library?

@nebulon42
Copy link
Contributor Author

This could work as shops have a quite distinct colour. But if we succeed in creating two different recognisable icons this would be better. See also #1228 (comment) for remaining issues I have with an updated version of the library icon.

@matkoniecz
Copy link
Contributor

@nebulon42 It would be better to avoid reordering names in defition of shop name.

On topic of adding new icon - I suppport it, bookstores are common shop type. Currently most mappers in Poland specify that shop is bookstore, as otherwise in default rendering it is impossible to guess shop type. Separate icon should fix the biggest source of this poor tagging.

@nebulon42
Copy link
Contributor Author

It would be better to avoid reordering names in defition of shop name.

What issues do you see with that? I thought it would be better to have it ordered alphabetically for quick overview.

@matkoniecz
Copy link
Contributor

What issues do you see with that? I thought it would be better to have it ordered alphabetically for quick overview.

Maybe, but it would be better as a separate commit - or even better a separate PR. This way it is easier to check and ensure that nothing goes wrong.

@daganzdaanda
Copy link

Nice icon!
I think the transparency is really needed here, you couldn't make the "pages" solid, and you couldn't leave them empty.

@nebulon42
Copy link
Contributor Author

removed reordering of shop name definitions.
@daganzdaanda If you look at the icon it is not really transparent, but looks like it at this size. It works quite well, though.

@matthijsmelissen
Copy link
Collaborator

@gravitystorm This also needs a strategic decision. Do we want to add more icons for shoptypes we currently only render as dot? Personally I support more icons as long as they are rezognizable, but I know you are more hesitant on this aspect.

@matkoniecz
Copy link
Contributor

@gravitystorm @pnorman What is your opinion? Is anybody opposed to adding more icons for shops?

Currently I am planning to merge it in a near future.

@kocio-pl
Copy link
Collaborator

I would love to add special icon for every possible type - visual hints work very good. Regarding this icon: @daganzdaanda is right about making it more solid, but given the choice current proposition or just a standard dot, I would take the first one for sure. Not visually perfect, but still recognizable.

@matthijsmelissen
Copy link
Collaborator

Nobody opposed, so as far as I'm concerned we merge this.

@nebulon42
Copy link
Contributor Author

Updated.

@pnorman
Copy link
Collaborator

pnorman commented Mar 17, 2015

I'm iffy on adding more icons. Slightly less so on shop icons.

@kocio-pl
Copy link
Collaborator

Why? If they aren't visible too early, I see no problem with more icons. As a matter of fact in micromapping we lack many of them - including shops, but if we care only for them, we would be not much better than Google Maps with their clear commercial attitude.

@matkoniecz
Copy link
Contributor

@nebulon42 Can you rebase it? I want to merge this PR.

@matkoniecz
Copy link
Contributor

@nebulon42 Also, why yaml and mml files are modified?

EDIT: I see, it is necessary as currently shop=books is currently translated to shop=other in SQL query.

@nebulon42
Copy link
Contributor Author

Rebased.

matkoniecz added a commit that referenced this pull request Mar 20, 2015
@matkoniecz matkoniecz merged commit 8f91dab into gravitystorm:master Mar 20, 2015
@matkoniecz
Copy link
Contributor

Thanks!

@nebulon42 nebulon42 deleted the books branch March 20, 2015 14:56
@kocio-pl
Copy link
Collaborator

Something probably went wrong a bit, since the icon seems to be rendered for areas:

https://www.openstreetmap.org/way/289091540

but not for the nodes (just created, so that can't be old dot):

http://www.openstreetmap.org/node/3410976366

@nebulon42
Copy link
Contributor Author

@kocio-pl There was a problem with the merge, which got fixed by now, but, unfortunately, didn't make it into the current release (yet).

edit: for clarification, not this merge, but the merge of #1383.

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.

6 participants