-
Notifications
You must be signed in to change notification settings - Fork 170
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 camp_site field to tourism=camp_site #1286
Conversation
🍱 You can preview the tagging presets of this pull request here. |
71c2a21
to
1ad7b36
Compare
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.
Thanks for this! I would only say that the strings are a bit too long for the combo field labels (the labels would often be truncated). See inline for two possible ways to shorten them.
…o to description Co-authored-by: Martin Raifer <martin@raifer.tech>
Thanks, I have committed your second suggestion. |
Hi, is anything else required? I am not quire sure how the Github PR review process works - I committed one of your suggestions, but it's still showing as "changes requested". Please let me know if there is anything else you would like me to do :-) |
Testing Looks good to me. ![]() Notes I found quite a few camp sites that do not have a preset when viewed in iD Query https://overpass-turbo.eu/s/1PW2 I wonder if that might be something to look into with a MapRoulette challenge … |
In general, #1286 (comment) was the right next step and then wait for someone to find the time to do a second review. For me, a review goes a lot faster the more I understand what is going on and have links (and screenshots) provided by others from the community to quickly go where I should look… Sometimes those are ideal with before and after views (screenshot/links). I tried going making this more formal with the new PR template that you will see with your next PR. |
@tyrasd I think you will need to have to squash-merge this. I cannot for some reason. It looks like once you reviewed it, you also have to approve it? I expected my review to be enough in combination with your review comment being "resolved" but it seems not… This is what the UI looks like for me: ![]() ![]() @osmuser63783 we will have to wait for Martin to merge this one. |
Thanks! The ones in your Overpass query have a lifecycle prefix, in this case The one in the second link has |
Btw when I created the PR in a branch in my fork, I accidentally based in on the wrong version of the repo. So I rebased it, which involved the force-pushing that you can see here in the PR history. I guess the moment you squash everything into one commit this doesn't matter? |
All good IMO. Force-push is always OK in those PRs IMO.
Yes. I would merge commit in cases where the commits have a meaningful structure and its helpful to look at them separately. Otherwise the squash is the quick way to cleanup the PR. |
This pull request is ready for review following a community forum discussion on the topic.
Description, Motivation & Context
tourism=camp_site
is used to map anything from a signposted spot where a tent can be pitched, without any facilities, to luxury glamping sites. The proposal forcamp_site=
which introduced a categorisation scheme was accepted in 2015. In a community forum discussion aboutcamp_site=basic
it was pointed out that this tag isn't included in the iD preset, so here's a PR for adding it.Testing help…
Overpass Turbo link