Crossings layer for browse page #343
Conversation
Begin to work on tooltip stuff Clickable crossing tooltips
|
|
||
| export let onClick = originalOnClick; | ||
|
|
||
| $: clickable = onClick !== originalOnClick; |
There was a problem hiding this comment.
This is a bit funky I'll admit, might be a better way to do
There was a problem hiding this comment.
My preference is to not have this indirection at all (see above). But if we keep it, this doesn't need to be reactive, because InteractiveLayer and such don't expect things like this to change after creation. It could just be a regular let clickable = .... calculated once.
| @@ -38,11 +42,6 @@ | |||
| // TODO Outline? | |||
| }); | |||
| let show = false; | |||
|
|
|||
| function tooltip(feature: MapGeoJSONFeature): string { | |||
There was a problem hiding this comment.
Although we didn't need it before, I definitely think it makes more sense for the specific layer control to determine how the tooltip is constructed
| > | ||
| <p> | ||
| This shows <ExternalLink | ||
| href="https://wiki.openstreetmap.org/wiki/Tag:crossing" |
There was a problem hiding this comment.
Link is broken, do you mean https://wiki.openstreetmap.org/wiki/Key:crossing?
| @@ -0,0 +1,64 @@ | |||
| <script lang="ts"> | |||
| import { circleRadius } from "colors"; | |||
There was a problem hiding this comment.
The dots are hard to hover on, even at tiny zooms. How do we feel about something like ["interpolate", ["linear"], ["zoom"], 1, 2, 8, 3, 13, 10] from the cycle parking layer? (In one of the other PRs out right now, I refactored another of these zoom-dependent patterns -- we could do the same). Also related to #326 more generally for UX at some zoom levels
| initialTooltipSentences.get(crossingTypeString); | ||
| if (initialTooltipSentence) result = initialTooltipSentence; | ||
|
|
||
| return result + `. More information can be found by clicking.`; |
There was a problem hiding this comment.
I think you need to put the entire thing in a <p> so govuk styles apply. The text is small right now.
| initialTooltipSentences.get(crossingTypeString); | ||
| if (initialTooltipSentence) result = initialTooltipSentence; | ||
|
|
||
| return result + `. More information can be found by clicking.`; |
There was a problem hiding this comment.
"Click for details" is shorter and direct?
| ["island", "Crossing with an island"], | ||
| [ | ||
| "informal", | ||
| "Infomral crossing where there crossing is not intended by the municipality but there is an obviously desire line", |
There was a problem hiding this comment.
Typos and odd grammar...
"Informal crossing with an obvious desire line, but no official infrastructure to support it" ?
| const name = "crossings"; | ||
|
|
||
| function tooltip(feature: MapGeoJSONFeature): string { | ||
| const crossingTypeString: string = feature.properties["crossing"]; |
There was a problem hiding this comment.
Does type inference not figure out string here? Also you can do properties.crossing to be more consistent with other code
| initialTooltipSentences.get(crossingTypeString); | ||
| if (initialTooltipSentence) result = initialTooltipSentence; | ||
|
|
||
| return result + `. More information can be found by clicking.`; |
There was a problem hiding this comment.
We've been using ${result}. Foo bar xyz for string interpolation, not concat generally, in this codebase
| import { circleRadius } from "colors"; | ||
| import { ExternalLink } from "lib/common"; | ||
| import type { MapGeoJSONFeature } from "maplibre-gl"; | ||
| import crossingsUrl from "../../../../assets/crossings.geojson?url"; |
There was a problem hiding this comment.
Can switch to pmtiles in the usual place now
There was a problem hiding this comment.
I don't think this component is a useful indirection, and some of the awkwardness plumbing through different options for railways vs crossings comes up here. CycleParkingLayerControl.svelte is a simple and short example not using the indirection.
|
|
||
| export let onClick = originalOnClick; | ||
|
|
||
| $: clickable = onClick !== originalOnClick; |
There was a problem hiding this comment.
My preference is to not have this indirection at all (see above). But if we keep it, this doesn't need to be reactive, because InteractiveLayer and such don't expect things like this to change after creation. It could just be a regular let clickable = .... calculated once.
assets/crossings.geojson
Outdated
|
|
||
| function tooltip(feature: MapGeoJSONFeature): string { | ||
| const crossingTypeString = feature.properties.crossing; | ||
| if (crossingTypeString === "no") |
There was a problem hiding this comment.
Why is this special and not in the mapping below? (and in case my other comment got lost, the style of using if without braces is different than everywhere else in the codebase. prettier doesn't change it right now, so just be aware it introduces inconsistency
| const crossingTypeString = feature.properties.crossing; | ||
| if (crossingTypeString === "no") | ||
| return "Location where crossing is impossible/illegal but where there is a clear desire line to cross."; | ||
| let result = "Crossing with the following features."; |
There was a problem hiding this comment.
This looks out-of-date and almost never used. I think you can just change all of this code to do descriptions.get(crossingType) ?? crossingType
| {tooltip} | ||
| {show} | ||
| clickable={false} | ||
| on:click={() => {}} |
Crossings with click-through to OSM. Needs changes to live hosting but this should be pretty representative