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

Create tagging schema for "Barangay Hall" #932

Closed
wants to merge 9 commits into from

Conversation

govvin
Copy link
Contributor

@govvin govvin commented Jun 22, 2023

Create tagging schema for "Barangay Hall".

A "barangay" in the Philippines is the smallest administrative government unit.

According to taginfo, "barangay" is the third most used value for townhall:type, and one of the most popular POI added by mappers from the Philippines.

I'm new to adding id tagging schemas, so please be gentle with me. :) Feedback is welcome!

Add term "barangay."
Create tagging schema for "Barangay Hall".

A "barangay" in the Philippines is the smallest administrative government unit.

According to [taginfo](https://taginfo.openstreetmap.org/tags/townhall%3Atype=barangay#overview), "barangay" is the third most used value for `townhall:type`, and one of the most popular POI added by mappers from the Philippines.

I'm new to adding id tagging schemas, so please be gentle with me. :) Feedback is welcome!
@github-actions
Copy link

🍱 Preview the tagging presets of this pull request here: https://pr-932--ideditor-presets-preview.netlify.app/id/dist/#locale=en.

tyrasd
tyrasd previously requested changes Jun 23, 2023
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

Hi. Thanks for this suggestion. It looks quite good already, only a few minor changes would be required: As this is a regional preset, the file name should have a suffix indicating the region it is valid for (i.e. data/…/barangay-PH.json). A few further comments are below:

data/presets/amenity/townhall.json Outdated Show resolved Hide resolved
data/presets/amenity/townhall/barangay.json Outdated Show resolved Hide resolved
data/presets/amenity/townhall/barangay.json Outdated Show resolved Hide resolved
data/presets/amenity/townhall/barangay.json Outdated Show resolved Hide resolved
@tyrasd
Copy link
Member

tyrasd commented Jun 23, 2023

ah, one last suggestion: we could split up the tags section of the preset into two parts, like this:

    "tags": {
        "amenity": "townhall",
        "townhall:type": "barangay"
    },
    "addTags": {
        "amenity": "townhall",
        "townhall:type": "barangay",
        "admin_level": "10"
    },

This would have the benefit that if the admin_level tag is missing on an existing OSM object, iD would suggest to add it.

govvin and others added 6 commits June 24, 2023 05:54
Remove "barangay" from the term section.
Incorporate recommended changes by reviewers, and additional updates:
* remove icon
* add "barrio hall" in terms section
Restore a comma I accidentally removed earlier.
remove extra space

Co-authored-by: Martin Raifer <martin@raifer.tech>
Remove superfluous "barangay hall" from terms section
Amend reference to amenity=townhall

Co-authored-by: Martin Raifer <martin@raifer.tech>
Copy link
Contributor Author

@govvin govvin left a comment

Choose a reason for hiding this comment

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

Revised the PR to incorporate the changes suggested by reviewers.

Revise tag section , and include "addTags"
@govvin govvin requested a review from tyrasd June 23, 2023 22:10
@tyrasd tyrasd dismissed their stale review June 27, 2023 12:57

no further changes needed

@tyrasd
Copy link
Member

tyrasd commented Jun 27, 2023

Thank you! This has now been merged as 699e10f.

@tyrasd tyrasd closed this Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants