-
Notifications
You must be signed in to change notification settings - Fork 164
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
Conversation
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. |
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. |
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. |
022056b
to
2fc8b7c
Compare
Use a combo instead of a checkbox in order to provide better information about the values and their meaning. openstreetmap#1226
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: 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. |
Hey @bompstable I like the idea of making the labels help with explaining the values.
However, we should be able to do the same while keeping the more convenient
I don't have sufficient permissions to edit it but you should be able to via the |
Use labelled values in a checkbox to instead of the more complicated combo. Resolves openstreetmap#1226
ec95040
to
ca42e3f
Compare
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. |
🍱 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.
There was a problem hiding this 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.
@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. |
Resolves #1226