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

Community centre #1200

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

tiptoptom
Copy link
Contributor

CC: #1015 #605

Copy link

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

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.

@tiptoptom in general this looks good. Could you please add some context to your PR to make it easer to review and also add some example links to those places using the preview that was linked in the PR by the github action. Please add usage stats on the given presets as well.

For example, why did you add separate presets per center-type? They all use the same fields and icon of their parent, so we might as well just use the parent + the new field?

One specific think I wonder is: You reference #1015 which talks about https://wiki.openstreetmap.org/wiki/Key:community_centre:for but your PR does not use this tag. Is this because you consider this something to be added later or are there other reasons?

Thanks for this PR!


Update: Two more things we need to check

@tiptoptom
Copy link
Contributor Author

For example, why did you add separate presets per center-type? They all use the same fields and icon of their parent, so we might as well just use the parent + the new field?

I have made various presets, as some of the names have little to do with "community center". For example, a club home of a sports club would not be called a community center but a club home.

@tiptoptom
Copy link
Contributor Author

One specific think I wonder is: You reference #1015 which talks about https://wiki.openstreetmap.org/wiki/Key:community_centre:for but your PR does not use this tag. Is this because you consider this something to be added later or are there other reasons?

I just wanted to put them in copy, as it is something similar.

@tiptoptom
Copy link
Contributor Author

Please add usage stats on the given presets as well.

2024-06-07_23-27
https://taghistory.raifer.tech/#***/amenity/community_centre&***/community_centre/club_home&***/community_centre/cultural_centre&***/community_centre/community_hall&***/community_centre/family_centre&***/community_centre/parish_hall&***/community_centre/village_hall


What does the search look like with those presets, are there duplicated results?

image

@tordans
Copy link
Collaborator

tordans commented Jun 8, 2024

@tiptoptom do you know more about the values that the "most used values" feature of the dropdown shows?
Eg. is club_house the same as the club_home.
It is always a bit confusing for users if those values are visible in the list but not explained/translated but others are.

image

@tordans
Copy link
Collaborator

tordans commented Jun 8, 2024

@tiptoptom One thing we should do before merging is improving the info-i text:

Eg image links to https://wiki.openstreetmap.org/wiki/Item:Q6740
But it should link to the specific sub tag.

You might need to add new wikidata items for those. Or add an explicit reference https://github.com/ideditor/schema-builder?tab=readme-ov-file#reference to the sub presets.

@tordans
Copy link
Collaborator

tordans commented Jun 8, 2024

@tiptoptom what are your thoughts on adding the sub-group-dropdown to the sub-presets as well?
I always find it a bit weird when I come from a preset, select a sub-preset via dropdown and then the dropdown disappears. It is hard to understand how to go back than. – This is not an issue if the primary usage will be selecting the sub-preset via search. However your graph showed a lot of usage without sub tags, so the first case is one that will happen a lot, right?

Right now it looks like this. The additional dropdown could be the first item on this fieldset
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants