-
Notifications
You must be signed in to change notification settings - Fork 4
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit funky I'll admit, might be a better way to do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can switch to pmtiles in the usual place now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good after a few more fixes. Nice!
|
||
function tooltip(feature: MapGeoJSONFeature): string { | ||
const crossingTypeString = feature.properties.crossing; | ||
if (crossingTypeString === "no") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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={() => {}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this
Crossings with click-through to OSM. Needs changes to live hosting but this should be pretty representative