MapComplete/MapComplete
MapComplete/MapComplete
3
5
Fork 6
Code Issues 172 Pull requests 2 Activity Actions

add sauna theme #2382

Open
Osmwithspace wants to merge 4 commits from Osmwithspace:sauna into develop
pull from: Osmwithspace:sauna
merge into: MapComplete:develop
Conversation 3 Commits 4 Files changed 6 +416
Osmwithspace commented 2025-04-18 10:28:32 +02:00
Contributor
Copy link

#2376

Opening a pull request on MapComplete

Hey! Thanks for opening a pull request on Mapcomplete! This probably means you want to add a new theme - if so, please
follow the checklist below. If this pull request is for some other issue, please ignore the template.

Adding your new theme

Thanks for taking the time to create your theme and to add it to the main repository!

To making merging smooth, please make sure that each of the following conditions is met:

  • The codebase is GPL-licensed. By opening a pull request, the new theme will be GPL too
  • All images are included in the pull request and no images are loaded from an external service (e.g. Wikipedia)
  • The guidelines on how to make your own theme
    are read and followed
#2376 Opening a pull request on MapComplete ===================================== Hey! Thanks for opening a pull request on Mapcomplete! This probably means you want to add a new theme - if so, please follow the checklist below. If this pull request is for some other issue, please ignore the template. Adding your new theme --------------------- Thanks for taking the time to create your theme and to add it to the main repository! To making merging smooth, please make sure that each of the following conditions is met: - [x] The codebase is GPL-licensed. By opening a pull request, the new theme will be GPL too - [x] All images are included in the pull request and no images are loaded from an external service (e.g. Wikipedia) - [x] The [guidelines on how to make your own theme](https://source.mapcomplete.org/MapComplete/MapComplete/src/branch/master/Docs/Making_Your_Own_Theme.md) are read and followed
Osmwithspace added 3 commits 2025-04-18 10:28:32 +02:00
add sauna theme
Some checks failed
/ deploy_on_hosted (pull_request) Failing after 4m22s
Details
718472de4f
pietervdvn reviewed 2025-04-18 14:36:12 +02:00
assets/layers/sauna/sauna.json
@ -0,0 +20,4 @@
"opening_hours",
{
"id": "charge_cost_rewritten",
"rewrite": {
pietervdvn commented 2025-04-18 14:36:12 +02:00
Owner
Copy link

Wow, you are using a 'rewrite'-block. That is pretty neat

Wow, you are using a 'rewrite'-block. That is pretty neat
❤️ 1
Osmwithspace marked this conversation as resolved
pietervdvn reviewed 2025-04-18 14:41:58 +02:00
assets/layers/sauna/sauna.json
@ -0,0 +98,4 @@
"source": {
"osmTags": {
"and": [
"leisure~.*sauna.*",
pietervdvn commented 2025-04-18 14:41:58 +02:00
Owner
Copy link

The 'source' is a bit weird. First of all, leisure~.*sauna.* will give all items which have a substring sauna in the leisure key.

Second, it seems like you are only selecting sauna's which have at least some item missing. This means that someone might add a sauna, fill out all details close MC. Upon reopening MC, the sauna they added will not show up again, as all information is known. This will cause them to add it again and to loose confidence in MapComplete, as "the data that they contributed dissappeared".

Keep in mind that MapComplete is not a todo app like streetcomplete, but also a 'browse OSM data'-app.

The 'source' is a bit weird. First of all, `leisure~.*sauna.*` will give all items which have a substring `sauna` in the leisure key. Second, it seems like you are _only_ selecting sauna's which have at least some item _missing_. This means that someone might add a sauna, fill out all details close MC. Upon reopening MC, the sauna they added will not show up again, as all information is known. This will cause them to add it again and to loose confidence in MapComplete, as "the data that they contributed dissappeared". Keep in mind that MapComplete is _not_ a todo app like streetcomplete, but also a 'browse OSM data'-app.
Osmwithspace commented 2025-04-18 20:02:58 +02:00
Author
Contributor
Copy link

The 'source' is a bit weird. First of all, leisure~.*sauna.* will give all items which have a substring sauna in the leisure key.

It's a very rare case, but I want to cover leisure=water_park;sauna for example.

Second, it seems like you are only selecting sauna's which have at least some item missing.

No, it's the other way around. leisure=sauna unfortunately has the problem that it's not clear whether it's a single sauna as part of a sauna area or an entire sauna area. Since I only want to display entire sauna areas, I filter in the sauna layer by opening_hours, website or phone.

This will cause them to add it again

Therefore, I want to prevent the addition of POIs in this theme.

I know that's not quite clean, but I think it's an acceptable way of dealing with the current situation.

> The 'source' is a bit weird. First of all, `leisure~.*sauna.*` will give all items which have a substring `sauna` in the leisure key. It's a very rare case, but I want to cover `leisure=water_park;sauna` for example. > Second, it seems like you are _only_ selecting sauna's which have at least some item _missing_. No, it's the other way around. `leisure=sauna` unfortunately has the problem that it's not clear whether it's a single sauna as part of a sauna area or an entire sauna area. Since I only want to display entire sauna areas, I filter in the `sauna` layer by `opening_hours`, `website` or `phone`. > This will cause them to add it again Therefore, I want to prevent the addition of POIs in this theme. I know that's not quite clean, but I think it's an acceptable way of dealing with the current situation.
pietervdvn commented 2025-04-20 01:38:29 +02:00
Owner
Copy link

It's a very rare case, but I want to cover leisure=water_park;sauna for example.

There were 5 leisure=water_park;sauna in OSM, I did retag them all. It happens that, when I make a theme, I do some initial data cleanup first too.

Since I only want to display entire sauna areas, I filter in the sauna layer by opening_hours, website or phone.

Hmm, this is an interesting case. Your approach ain't foolproof, as someone might have tagged it on both the 'area' and the individual sauna parts.

Maybe an approach with loading the outlines of swimming pool facilities, water parks, ... too and then removing sauna's within such amenity (with some metatagging) might work. Could you link a few examples?

Also: how do we deal with private sauna's in someones backyard?

Therefore, I want to prevent the addition of POIs in this theme.

Oh, I didn't catch that. The MapComplete paradigm is to allow addition as much as possible too. Otherwise, I'll have an issue pretty soon stating "cannot add new sauna" ;)
So, I can exceptionally allow this, but only if all else fails.

> It's a very rare case, but I want to cover leisure=water_park;sauna for example. There were 5 `leisure=water_park;sauna` in OSM, I did retag them all. It happens that, when I make a theme, I do some initial data cleanup first too. > Since I only want to display entire sauna areas, I filter in the sauna layer by opening_hours, website or phone. Hmm, this is an interesting case. Your approach ain't foolproof, as someone might have tagged it on both the 'area' and the individual sauna parts. Maybe an approach with loading the outlines of swimming pool facilities, water parks, ... too and then removing sauna's _within_ such amenity (with some metatagging) might work. Could you link a few examples? Also: how do we deal with private sauna's in someones backyard? > Therefore, I want to prevent the addition of POIs in this theme. Oh, I didn't catch that. The MapComplete paradigm is to allow addition as much as possible too. Otherwise, I'll have an issue pretty soon stating "cannot add new sauna" ;) So, I can exceptionally allow this, but only if all else fails.
pietervdvn reviewed 2025-04-18 14:43:08 +02:00
assets/layers/sauna/sauna.json
@ -0,0 +120,4 @@
"render": {
"en": "{name}"
}
}
pietervdvn commented 2025-04-18 14:43:08 +02:00
Owner
Copy link

Please, also add an allowMove block. These aren't added by default in Studio, but it is required for official themes. It is also what caused the build to fail.

The simplest way is to add "allowMove": true

Please, also add an `allowMove` block. These aren't added by default in Studio, but it is required for official themes. It is also what caused the build to fail. The simplest way is to add `"allowMove": true`
Osmwithspace marked this conversation as resolved
pietervdvn reviewed 2025-04-18 14:43:32 +02:00
assets/layers/sauna/sauna.json
@ -0,0 +86,4 @@
]
},
"contact",
"defibrillator.defibrillator-fixme"
pietervdvn commented 2025-04-18 14:43:32 +02:00
Owner
Copy link

You are reusing the 'fixme' for 'defibrillators'. This results in a question 'are there any other things you want to say about this defibrillator ?'. That is pretty confusing as this is about sauna's...

In general, I try to not use 'fixme' as it burdens more experienced mappers and tends to make that users just restate some facts that are already captured in a tag.

You are reusing the 'fixme' for 'defibrillators'. This results in a question 'are there any other things you want to say about this *defibrillator* ?'. That is pretty confusing as this is about sauna's... In general, I try to not use 'fixme' as it burdens more experienced mappers and tends to make that users just restate some facts that are already captured in a tag.
Osmwithspace commented 2025-04-18 21:51:50 +02:00
Author
Contributor
Copy link

You are reusing the 'fixme' for 'defibrillators'. This results in a question 'are there any other things you want to say about this defibrillator ?'. That is pretty confusing as this is about sauna's...

As far as i see, the sentences are not specific for defibrillators.

In general, I try to not use 'fixme' as it burdens more experienced mappers and tends to make that users just restate some facts that are already captured in a tag.

In fact, I'm not entirely happy with it either. I wanted to offer a way to make low-threshold posts/comments when you have reached the functional limits of the theme.

> You are reusing the 'fixme' for 'defibrillators'. This results in a question 'are there any other things you want to say about this _defibrillator_ ?'. That is pretty confusing as this is about sauna's... As far as i see, the [sentences](https://source.mapcomplete.org/MapComplete/MapComplete/src/commit/5515114be95bd442d682c01818e0fcd48507c455/assets/layers/defibrillator/defibrillator.json#L688-L718) are not specific for defibrillators. > In general, I try to not use 'fixme' as it burdens more experienced mappers and tends to make that users just restate some facts that are already captured in a tag. In fact, I'm not entirely happy with it either. I wanted to offer a way to make low-threshold posts/comments when you have reached the functional limits of the theme.
pietervdvn commented 2025-04-22 01:26:40 +02:00
Owner
Copy link

As far as i see, the sentences are not specific for defibrillators.

The dutch (nl) one is ;)
In any case, it is a bad habit of reusing this tagrendering from an unrelated layer. I propose you move the "fixme" into the "questions/questions.json"-file (and drop the dutch text). Then, you can reuse it.

In fact, I'm not entirely happy with it either. I wanted to offer a way to make low-threshold posts/comments when you have reached the functional limits of the theme.

Then maybe note=* or description=* is more fitting? Anyway, fixme=* is acceptable as well if you insist. Related: #1678

Those free text fields are also quite good indicators of what the functional limits are and serve as great inspiration for new questions.

> As far as i see, the sentences are not specific for defibrillators. The dutch (`nl`) one is ;) In any case, it is a bad habit of reusing this tagrendering from an unrelated layer. I propose you move the "fixme" into the "questions/questions.json"-file (and drop the dutch text). Then, you can reuse it. > In fact, I'm not entirely happy with it either. I wanted to offer a way to make low-threshold posts/comments when you have reached the functional limits of the theme. Then maybe `note=*` or `description=*` is more fitting? Anyway, `fixme=*` is acceptable as well if you insist. Related: #1678 Those free text fields are also quite good indicators of what the functional limits are and serve as great inspiration for new questions.
pietervdvn commented 2025-04-18 15:40:31 +02:00
Owner
Copy link

I will review the 'sauna-at-amenity'-layer on, but might, in the meantime, also work on some code to make this type of question reuse easier (i.e. to automatically 'prefix' every key)

I will review the 'sauna-at-amenity'-layer on, but might, in the meantime, also work on some code to make this type of question reuse easier (i.e. to automatically 'prefix' every key)
👀 1
Osmwithspace added 1 commit 2025-04-18 21:31:23 +02:00
add "allowMove": true
Some checks failed
/ deploy_on_hosted (pull_request) Failing after 4m33s
Details
b06f937c5b
pietervdvn commented 2025-04-20 01:49:00 +02:00
Owner
Copy link

Another small issue (which causes the build to block) is that sauna_at_leisure should be in its own directory in layers/sauna_at_leisure

Another small issue (which causes the build to block) is that `sauna_at_leisure` should be in its own directory in `layers/sauna_at_leisure`
pietervdvn commented 2025-04-22 01:30:13 +02:00
Owner
Copy link

I've created a new way to reuse tagRenderings where you can set a prefix. It can be used as following:

{
  "builtin": "sauna.*",
  "override": {
    "condition":{"and"+: "sauna=yes"}
  }
  "prefix": "sauna"
  }

This will import all questions from the sauna layer and modify all keys to have sauna: in front of it. The only thing you still need to create is a question if there is a sauna.

Again; highly experimental!

I've created a new way to reuse tagRenderings where you can set a prefix. It can be used as following: ```json { "builtin": "sauna.*", "override": { "condition":{"and"+: "sauna=yes"} } "prefix": "sauna" } ``` This will import _all_ questions from the sauna layer and modify all keys to have `sauna:` in front of it. The only thing you still need to create is a question _if_ there is a sauna. Again; highly experimental!
Some checks failed
/ deploy_on_hosted (pull_request) Failing after 4m33s
Details
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u sauna:Osmwithspace-sauna
git checkout Osmwithspace-sauna

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout develop
git merge --no-ff Osmwithspace-sauna
git checkout Osmwithspace-sauna
git rebase develop
git checkout develop
git merge --ff-only Osmwithspace-sauna
git checkout Osmwithspace-sauna
git rebase develop
git checkout develop
git merge --no-ff Osmwithspace-sauna
git checkout develop
git merge --squash Osmwithspace-sauna
git checkout develop
git merge --ff-only Osmwithspace-sauna
git checkout develop
git merge Osmwithspace-sauna
git push origin develop
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Android-shell

Label for issues related to the android shell

  
awaiting feedback

We are waiting for the OP to get back to us, e.g. with an example

  
awaiting fix confirmation

We are waiting for OP to confirm that the fix worked for them (or will close when they don't reply)

  
bug

Something isn't working

  
enhancement

New feature or request

  
Help Wanted

Extra attention is needed

  
high-priority

It's urgent!

  
huge

A huge task, preferably to do with funding

  
low-priority

Might be fixed one day... or not

  
Meta

Something that cannot be fixed by code

  
NLNet

Will be developed, funded by NLNet

  
OSOC21:Cycling-OVL

A feature specific for this project

  
Performance

Make things faster or leaner

  
question

Further information is requested

  
search-ui-enhancement

Related to the search functionality

  
Studio

Graphical user interface to create or edit themes and layers

  
Tailwind

Something related to the CSS-framework

  
Themes

A new theme idea or improvement to an already existing theme

  
UI

Related to the user interface

  
upstream-issue

An issue that should be fixed in an upstram library

  
usertest

Issues surfaced during usertests

No labels
Android-shell
awaiting feedback
awaiting fix confirmation
bug
enhancement
Help Wanted
high-priority
huge
low-priority
Meta
NLNet
OSOC21:Cycling-OVL
Performance
question
search-ui-enhancement
Studio
Tailwind
Themes
UI
upstream-issue
usertest
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: MapComplete/MapComplete#2382
No description provided.
Delete branch "Osmwithspace:sauna"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?