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 emergency=fire_service_inlet #806

Merged
merged 16 commits into from
Mar 14, 2023
Merged

Add emergency=fire_service_inlet #806

merged 16 commits into from
Mar 14, 2023

Conversation

tiptoptom
Copy link
Contributor

Maybe there is a better icon for this.

@tiptoptom
Copy link
Contributor Author

I don't know how to solve the conflicts. Can you tell me, what's missing?
https://wiki.openstreetmap.org/wiki/Tag:emergency%3Dfire_service_inlet#Tagging

tyrasd
tyrasd previously requested changes Mar 6, 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.

The field fire_mains does not exist yet.1 You'd have to create the field in data/fields/fire_mains.json, in order to use it in the preset.

Footnotes

  1. This produces the error in the build&test script, see https://github.com/openstreetmap/id-tagging-schema/actions/runs/4335474805/jobs/7570094016

data/presets/emergency/fire_service_inlet.json Outdated Show resolved Hide resolved
data/presets/emergency/fire_service_inlet.json Outdated Show resolved Hide resolved
data/presets/emergency/fire_service_inlet.json Outdated Show resolved Hide resolved
@tyrasd
Copy link
Member

tyrasd commented Mar 6, 2023

PS: For reference: The tag was quite recently "approved" (see https://wiki.openstreetmap.org/w/index.php?oldid=2483791)

@tiptoptom
Copy link
Contributor Author

I think it should be fine now.

data/presets/emergency/fire_service_inlet.json Outdated Show resolved Hide resolved
data/fields/fire_mains.json Outdated Show resolved Hide resolved
data/presets/emergency/fire_service_inlet.json Outdated Show resolved Hide resolved
{
"key": "fire_mains",
"type": "combo",
"label": "Type",
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to clarify that this Type is only supposed to represent the non-sprinkler part of the inlet. Maybe just calling it Riser Inlet (or Standpipe) would be better?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is clear enough. emergency=fire_service_inlet is basically only about the inlet. So I think Riser Inlet or Standpipe would only lead to confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or maybe 'System-Type'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it in Network-Type. I think this should be clear now.

data/fields/fire_mains.json Outdated Show resolved Hide resolved
@tyrasd tyrasd dismissed their stale review March 7, 2023 11:49

requested changes look good, but further comments were made in the meantime which need to be clarified

@github-actions
Copy link

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

@tyrasd tyrasd merged commit 29130c5 into openstreetmap:main Mar 14, 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.

3 participants