Climbing theme improvements #2284

midgard · 2024-12-09T22:34:49+01:00

midgard commented

2024-12-09 22:34:49 +01:00

🆕 New questions about auto belay! Some improvements to wording and question conditions. Neatly packaged per commit, check those one by one!

midgard added 3 commits 2024-12-09 22:34:50 +01:00

Themes(climbing): Change conditions for gear rental questions f8cfcb9533

- Also recognize speed as enough to show the harness rental question
- Require answer on climbing styles before showing harness/belay device rental

Themes(climbing): Clarify questions about gear rental 91e204aade

Specify in the question that it's for gear to rent for use within the
gym.

Themes(climbing): Add auto belay tags

Build and validate PR (but don't deploy) / build (pull_request) Failing after 4m50s

Details

c26300bb75

I even documented these tags on the wiki!
https://wiki.openstreetmap.org/wiki/Climbing#Climbing_gym_services_and_facilities

pietervdvn reviewed 2024-12-09 22:49:00 +01:00

assets/layers/climbing_gym/climbing_gym.json Outdated

					
				@ -318,0 +334,4 @@

				    {

				      "id": "auto_belay_toprope",

				      "question": {

				        "en": "Are there auto belays for top roping here? (Excluding those for speed)",

pietervdvn commented

2024-12-09 22:49:00 +01:00

Use "questionHint" for extra remarks, such as excluding those for speed

Use "questionHint" for extra remarks, such as `excluding those for speed`

midgard marked this conversation as resolved

pietervdvn reviewed 2024-12-09 22:50:31 +01:00

assets/layers/climbing_gym/climbing_gym.json Outdated

					
				@ -318,0 +377,4 @@

				          }

				        },

				        {

				          "if": "climbing:autobelay:toprope~[0-9]+",

pietervdvn commented

2024-12-09 22:50:31 +01:00

If one sets a freeform with key, you should use render as fallback option:

"render":  {
            "en": "There are {climbing:autobelay:toprope} auto belay devices for top roping",
            "nl": "Er zijn {climbing:autobelay:toprope} autobelaytoestellen voor toprope"
          }

Remove this mapping, and add the render to the question

If one sets a `freeform` with key, you should use `render` as fallback option: ``` "render": { "en": "There are {climbing:autobelay:toprope} auto belay devices for top roping", "nl": "Er zijn {climbing:autobelay:toprope} autobelaytoestellen voor toprope" } ``` Remove this mapping, and add the `render` to the question

midgard marked this conversation as resolved

pietervdvn reviewed 2024-12-09 22:50:52 +01:00

assets/layers/climbing_gym/climbing_gym.json Outdated

					
				@ -318,0 +424,4 @@

				          }

				        },

				        {

				          "if": "climbing:autobelay:sport~[0-9]+",

pietervdvn commented

2024-12-09 22:50:52 +01:00

Same as above

midgard marked this conversation as resolved

pietervdvn commented

2024-12-09 22:52:41 +01:00

Looks like some very valuable fixes!

However, there are still a few technical nitpicks to solve, which'll remove this error message: ./assets/layers/climbing_gym/climbing_gym.json.tagRenderings.13: No 'render' defined,./assets/layers/climbing_gym/climbing_gym.json.tagRenderings.14: No 'render' defined

Looks like some very valuable fixes! However, there are still a few technical nitpicks to solve, which'll remove this error message: ./assets/layers/climbing_gym/climbing_gym.json.tagRenderings.13: No 'render' defined,./assets/layers/climbing_gym/climbing_gym.json.tagRenderings.14: No 'render' defined

pietervdvn requested changes 2024-12-09 22:53:09 +01:00

Dismissed

pietervdvn left a comment

There are still a few bugs - see the comments I left.

midgard force-pushed climbing-improvements from c26300bb75 to 5abd18cfc3

2024-12-10 01:43:08 +01:00

Compare

midgard force-pushed climbing-improvements from 5abd18cfc3 to 8e420f14ac

2024-12-10 01:56:08 +01:00

Compare

midgard force-pushed climbing-improvements from 8e420f14ac to f35bf01abb

2024-12-10 02:00:53 +01:00

Compare

midgard commented

2024-12-10 02:05:09 +01:00

Thanks for the review! I fixed the comments and force-pushed the fixed commits (and one new one about leisure=sports_hall, there are already 40 climbing sports_halls) to this PR's branch. I did a few force-pushes and a rebase so the Compare links above aren't very useful. (In the future I'll avoid force-pushing fixes and rebases at the same time). The differences can be seen here and here.

Previously I had tried to remember what command I needed to run but couldn't, and then had forgotten to test the build at all. (Woops.)

I now used npm run generate:layouts and it finished without warnings that seemed relevant, is that correct to test a change like this?

Thanks for the review! I fixed the comments and force-pushed the fixed commits (and one new one about `leisure=sports_hall`, there are already [40 climbing sports_halls](https://overpass-turbo.eu/s/1Vqe)) to this PR's branch. I did a few force-pushes and a rebase so the Compare links above aren't very useful. (In the future I'll avoid force-pushing fixes and rebases at the same time). The differences can be seen [here](https://source.mapcomplete.org/MapComplete/MapComplete/compare/5abd18cfc357293a11b19f2227ce5f9baa0ae4bd..f35bf01abbcefe750bb873503dcb7fa10d7fecde) and [here](https://source.mapcomplete.org/MapComplete/MapComplete/commit/f35bf01abbcefe750bb873503dcb7fa10d7fecde). Previously I had tried to remember what command I needed to run but couldn't, and then had forgotten to test the build at all. (Woops.) I now used `npm run generate:layouts` and it finished without warnings that seemed relevant, is that correct to test a change like this?

midgard requested review from pietervdvn 2024-12-10 02:13:09 +01:00

pietervdvn approved these changes 2024-12-10 16:50:39 +01:00

pietervdvn commented

2024-12-10 16:51:49 +01:00

Excellent! The build also passed: https://hosted.mapcomplete.org/2284/climbing.html?