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 camp_site field to tourism=camp_site #1286

Merged
merged 6 commits into from
Aug 20, 2024

Conversation

osmuser63783
Copy link
Contributor

@osmuser63783 osmuser63783 commented Jul 14, 2024

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 for camp_site= which introduced a categorisation scheme was accepted in 2015. In a community forum discussion about camp_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…

Screenshot 2024-07-14 at 14 17 13 Screenshot 2024-07-14 at 14 17 23

Overpass Turbo link

Copy link

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

@osmuser63783 osmuser63783 force-pushed the campsite branch 2 times, most recently from 71c2a21 to 1ad7b36 Compare July 14, 2024 12:01
Copy link
Member

@tyrasd tyrasd left a 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.

data/fields/camp_site.json Outdated Show resolved Hide resolved
…o to description

Co-authored-by: Martin Raifer <martin@raifer.tech>
@osmuser63783
Copy link
Contributor Author

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.

Thanks, I have committed your second suggestion.

@osmuser63783 osmuser63783 requested a review from tyrasd August 2, 2024 14:31
@osmuser63783
Copy link
Contributor Author

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 :-)

@tordans
Copy link
Collaborator

tordans commented Aug 19, 2024

Testing

Looks good to me.

image

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

https://pr-1286--ideditor-presets-preview.netlify.app/id/dist/#background=Bing&disable_features=boundaries&id=n1280097225&locale=en&map=19.09/50.54228/14.02116 for example.

I wonder if that might be something to look into with a MapRoulette challenge …

@tordans
Copy link
Collaborator

tordans commented Aug 19, 2024

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 :-)

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.

@tordans
Copy link
Collaborator

tordans commented Aug 19, 2024

@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…
Could you check the rules if there is something that can be adjusted?

This is what the UI looks like for me:

image image

@osmuser63783 we will have to wait for Martin to merge this one.

@osmuser63783
Copy link
Contributor Author

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

https://pr-1286--ideditor-presets-preview.netlify.app/id/dist/#background=Bing&disable_features=boundaries&id=n1280097225&locale=en&map=19.09/50.54228/14.02116 for example.

I wonder if that might be something to look into with a MapRoulette challenge …

Thanks!

The ones in your Overpass query have a lifecycle prefix, in this case demolished:tourism=camp_site, which iD doesn't support. So no action necessary there I think.

The one in the second link has camp_site=reception which is not part of the approved classification scheme for camp sites, just an unrelated use of the same key from an abandoned proposal.

@osmuser63783
Copy link
Contributor Author

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?

@tordans tordans merged commit 5d95be8 into openstreetmap:main Aug 20, 2024
5 checks passed
@tordans
Copy link
Collaborator

tordans commented Aug 20, 2024

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.

All good IMO. Force-push is always OK in those PRs IMO.

I guess the moment you squash everything into one commit this doesn't matter?

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.

@osmuser63783 osmuser63783 deleted the campsite branch August 20, 2024 14:23
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.

3 participants