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 armrest field to amenity=bench #1227

Merged
merged 4 commits into from
May 31, 2024

Conversation

bompstable
Copy link
Contributor

@bompstable bompstable commented May 17, 2024

Resolves #1226

@tordans
Copy link
Collaborator

tordans commented May 18, 2024

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
Copy link
Contributor Author

Have you seen #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
Copy link
Contributor Author

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.

Use a combo instead of a checkbox in order to provide better information
about the values and their meaning.

openstreetmap#1226
@bompstable
Copy link
Contributor Author

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
Copy link
Collaborator

tordans commented May 29, 2024

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.

Use labelled values in a checkbox to instead of the more complicated
combo.

Resolves openstreetmap#1226
@bompstable
Copy link
Contributor Author

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.

Copy link

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

Following the mindset that we want to show existing data but the use case for color is not big enough to nudge people to add the field.
Copy link
Collaborator

@tordans tordans left a comment

Choose a reason for hiding this comment

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

Tested in https://pr-1227--ideditor-presets-preview.netlify.app/id/dist/#background=Bing&disable_features=boundaries&id=n5586406049&locale=en&map=19.33/52.47640/13.44474 and looks good.

I moved the color field to moreFields so the default fields are those that we want to nudge people to fill out but the color is still visible whenever present.

@tordans tordans merged commit 87d7b90 into openstreetmap:main May 31, 2024
5 checks passed
@tordans
Copy link
Collaborator

tordans commented May 31, 2024

@bompstable thanks for the PR. I used it today to test my new merge permissions on this repo :) (see #1230, #1229).

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

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.

Add armrest as a default field for amenity=bench
3 participants