Skip to content

Add emergency=fire_service_inlet#806

Merged
tyrasd merged 16 commits intoopenstreetmap:mainfrom
tiptoptom:main
Mar 14, 2023
Merged

Add emergency=fire_service_inlet#806
tyrasd merged 16 commits intoopenstreetmap:mainfrom
tiptoptom:main

Conversation

@tiptoptom
Copy link
Copy Markdown
Contributor

Maybe there is a better icon for this.

@tiptoptom
Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread data/presets/emergency/fire_service_inlet.json Outdated
Comment thread data/presets/emergency/fire_service_inlet.json Outdated
Comment thread data/presets/emergency/fire_service_inlet.json Outdated
@tyrasd
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

I think it should be fine now.

Comment thread data/presets/emergency/fire_service_inlet.json Outdated
Comment thread data/fields/fire_mains.json Outdated
Comment thread data/presets/emergency/fire_service_inlet.json Outdated
Comment thread data/presets/emergency/fire_service_inlet.json Outdated
Comment thread data/fields/fire_mains.json Outdated
{
"key": "fire_mains",
"type": "combo",
"label": "Type",
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread data/fields/fire_mains.json Outdated
Comment thread data/fields/fire_sprinkler.json Outdated
@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
Copy Markdown

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

@tyrasd tyrasd added the new-field create a new field (see add-field for cases where field from presets is added to new entries) label Mar 14, 2023
@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

Labels

new-field create a new field (see add-field for cases where field from presets is added to new entries) new-preset

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants