-
-
Notifications
You must be signed in to change notification settings - Fork 20
add bench armrests quest #847
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
base: modified
Are you sure you want to change the base?
Conversation
mnalis
left a comment
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.
Congrats on your first PR @newplaylist47 !
I'm glad to hear debug APK worked and quest behaves as expected (if I understood correctly)
Could you change/move that string?
Other that that (and icon, but do not worry about it if it is not your forte) it looks good to me!
(P.S. I've also changed the text to "Fixes" as that is magic word that GitHub uses to automatically close the issue when PR is merged).
| """ | ||
| override val changesetComment = "Survey whether benches have armrests" | ||
| override val wikiLink = "Tag:amenity=bench" | ||
| override val icon = R.drawable.ic_quest_bench_poi |
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.
Do we perhaps need (slightly) different icon?
It might be somewhat disorienting/confusing if we solve backrest quest, and then the same icon pops up (with very similar text)...
Anyone good with vector icons?
|
|
||
|
|
||
| <!-- new quests --> | ||
| <string name="quest_bench_armrest_title">Does this bench have armrests?</string> |
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.
| <string name="quest_bench_armrest_title">Does this bench have armrests?</string> | |
| <string name="quest_bench_armrest_title">Does this bench have any armrest?</string> |
or something similar... because, according to to wiki, single armrest also makes it eligible for yes answer.
Also, I'd move it close to existing <!-- bench material --> string (and add a <!-- bench armrests --> heading)
Fixes #834