search

openstreetmap / id-tagging-schema

🆔🏷 The presets and other tagging data used by the iD editor
ISC License
137 stars 148 forks source link

Add armrest field to amenity=bench #1227

Closed bompstable closed 1 month ago

bompstable commented 1 month ago

Resolves openstreetmap/id-tagging-schema#1226

tordans commented 1 month ago

I think we should put this in the moreFields section. Personally I consider it a nice to have additional information but not crucial to the data.

What is our naming pattern the label of similar "may be one or many" fields? I think we should avoid the "/s" if possible.

bompstable commented 1 month ago

Have you seen https://github.com/openstreetmap/id-tagging-schema/issues/1226? I've tried to explain a bit more there.

Whether a bench has a backrest and armrests is important information to people with mobility issues. I would like to promote the gathering of armrest information alongside backrest, which is already included in the default fields. I don't believe this will happen if is "buried" under moreFields as it is far less discoverable as a datapoint to record.

bompstable commented 1 month ago

Other examples of "one or more" fields are footrest, handrest and handrail. These all use just the singular in the label. I think it would be okay to say just "armrest" if that is preferable.

Regarding whether this is a nice to have field, I would say it is more important as it is providing accessibility information for users of the bench. We already include material and colour in the default fields and I would argue that these are less important fields.

bompstable commented 1 month ago

I've updated this to use a combo instead of a checkbox for the armrest tag. This allows me to provide better information about how the tag is used and to avoid the "armrest(s)" issue. In iD, this looks as follows:

image

A similar approach was suggested for this in JOSM.

@tordans Are you able to update #1226 to add this PR? I forgot to include a reference to the issue when I first created this.

tordans commented 1 month ago

Hey @bompstable I like the idea of making the labels help with explaining the values.

I've updated this to use a combo instead of a checkbox for the armrest tag. This allows me to provide better information about how the tag is used and to avoid the "armrest(s)" issue. In iD, this looks as follows:

However, we should be able to do the same while keeping the more convenient checkbox field. According to https://github.com/ideditor/schema-builder?tab=readme-ov-file#strings and the linked example https://github.com/openstreetmap/id-tagging-schema/blob/2375a6b/data/fields/parcel_pickup.json#L5-L11 you can specify the same labels with the check field type.

@tordans Are you able to update #1226 to add this PR? I forgot to include a reference to the issue when I first created this.

I don't have sufficient permissions to edit it but you should be able to via the ... in the top right of the description box. However, mentioning the issue anywhere here already created a link to the issue. And when you use one of those keywords https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests the issue is automatically closed once this is merged.

bompstable commented 1 month ago

we should be able to do the same while keeping the more convenient checkbox field.

Thanks! I didn't pick up on that option when I read the docs. I've now changed it to use labelled checkboxes and agree that is better.

github-actions[bot] commented 1 month ago

:bento: You can preview the tagging presets of this pull request here.

tordans commented 1 month ago

@bompstable thanks for the PR. I used it today to test my new merge permissions on this repo :) (see https://github.com/openstreetmap/id-tagging-schema/pull/1230, https://github.com/openstreetmap/id-tagging-schema/pull/1229).

The change will gradually roll out to consumers of this schema whenever they update their software and create a new release.