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

Adding amenity=bbq icon #3133

Merged
merged 1 commit into from
Mar 22, 2018
Merged

Conversation

james2432
Copy link
Contributor

@james2432 james2432 commented Mar 19, 2018

Fixes #2996
From https://www.openstreetmap.org/node/4983372050#map=19/45.35162/-75.82804 (node):
image

From: https://www.openstreetmap.org/#map=19/43.31581/-70.92575 (way)
image
(Should we render ways with a border? or just the little icon?)

From: https://www.openstreetmap.org/#map=17/45.51983/-75.84958 (node with text in name)
image

@kocio-pl
Copy link
Collaborator

Should we render ways with a border? or just the little icon?

I guess icon is enough. If this is in the recreation zone, it's easy to add such area in parallel.

@james2432 james2432 changed the title Issue #2996 Adding amenity bbq icon Adding amenity bbq icon Mar 20, 2018
@kocio-pl kocio-pl changed the title Adding amenity bbq icon Adding amenity=bbq icon Mar 20, 2018
@@ -1906,7 +1913,8 @@
}
}

[feature = 'amenity_atm'][zoom >= 17] {
[feature = 'amenity_atm'][zoom >= 17],
[feature = 'amenity_bbq'][zoom >= 17] {
text-name: "[operator]";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name tag is used here, operator is not even visible in stats:

https://taginfo.openstreetmap.org/tags/amenity=bbq#combinations

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something went wrong and - just by looking at code - you render operator instead of name again.

@james2432 james2432 force-pushed the amenity_bbq branch 2 times, most recently from ae03e62 to dd18dac Compare March 21, 2018 01:28
@james2432
Copy link
Contributor Author

Replaced operator use by name, not sure why I was using operator (was thinking maybe bbq's usually are operated by someone vs have a name?)

@kocio-pl
Copy link
Collaborator

kocio-pl commented Mar 21, 2018

Nice, could you also render a way with a name? Also please test updated code on a node (it should read "Dunlop Picnic").

@kocio-pl
Copy link
Collaborator

After merging similar item (amenity=shower) you need to fix the conflicts in project.mml to make this PR mergable again.

@james2432
Copy link
Contributor Author

Fixed.

@kocio-pl
Copy link
Collaborator

It renders OK and I'm gonna merge it, but could you make it 1 commit (instead of 2)?

@matthijsmelissen
Copy link
Collaborator

@kocio-pl of course you can do that yourself as well.

@james2432
Copy link
Contributor Author

I'm working on it, I just dont have access to git lfs on windows machine so I need a linux environment to download it should be done within next 20 minutes

@james2432 james2432 force-pushed the amenity_bbq branch 2 times, most recently from 86f4762 to 0fef15f Compare March 22, 2018 13:43
@james2432
Copy link
Contributor Author

Done.

@kocio-pl
Copy link
Collaborator

Unfortunately we're back to the operator label (instead of a name label).

@james2432
Copy link
Contributor Author

Not sure what went wrong, possibly this computer didn't fetch my origin changes. Sorry about that

@kocio-pl
Copy link
Collaborator

No problem. Wait a moment, I test adv column now, so the conflicts will be back.

@james2432
Copy link
Contributor Author

Fixed the conflict issue with advertising

@kocio-pl kocio-pl merged commit 197ffcd into gravitystorm:master Mar 22, 2018
@kocio-pl
Copy link
Collaborator

Thanks!

We are getting more outdoor lately (shower, bbq, fire pit, aerialways, shrines, towers...). That's great, OSM data are really diverse.

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.

3 participants