-
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
Changes from 2 commits
c45eb87
d81b383
34c8906
f53af22
52ac424
9e877c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
<script lang="ts"> | ||
import { circleRadius } from "colors"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Can switch to pmtiles in the usual place now |
||
import PointAmenityLayerControl from "./PointAmenityLayerControl.svelte"; | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Does type inference not figure out |
||
if (crossingTypeString === "no") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 commentThe 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 |
||
const initialTooltipSentence = | ||
initialTooltipSentences.get(crossingTypeString); | ||
if (initialTooltipSentence) result = initialTooltipSentence; | ||
|
||
return ( | ||
result + | ||
`. More information can be found by clicking.` | ||
); | ||
} | ||
|
||
const initialTooltipSentences: Map<string, string> = new Map<string, string>([ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general in TS, I don't think you need to do |
||
["traffic_signals", "Signalised crossing"], | ||
["marked", "Crossing with no traffic signals"], | ||
["uncontrolled", "Crossing with no traffic signals"], | ||
["unmarked", "Crossing with no markings or signals"], | ||
["zebra", "Zebra crossing"], | ||
["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 commentThe 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" ? |
||
], | ||
]); | ||
|
||
function onClick(e: CustomEvent<MapGeoJSONFeature>) { | ||
window.open( | ||
`http://openstreetmap.org/node/${e.detail.properties.osm_id}`, | ||
"_blank" | ||
); | ||
} | ||
</script> | ||
|
||
<PointAmenityLayerControl | ||
{name} | ||
pluralNoun="Crossings" | ||
{circleRadius} | ||
url={crossingsUrl} | ||
{tooltip} | ||
{onClick} | ||
> | ||
<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 commentThe 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? |
||
> | ||
crossing | ||
</ExternalLink> data from OpenStreetMap (as of 9 August 2023). | ||
</p> | ||
|
||
<p> | ||
License: <ExternalLink href="https://www.openstreetmap.org/copyright"> | ||
Open Data Commons Open Database License | ||
</ExternalLink> | ||
</p> | ||
</PointAmenityLayerControl> |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,23 +12,27 @@ | |
// - The layer name, for layerZorder | ||
// - A color name in colors.ts | ||
export let name: string; | ||
// Uncapitalized | ||
export let singularNoun: string; | ||
// Capitalized | ||
export let pluralNoun: string; | ||
|
||
export let circleRadius: number; | ||
|
||
export let url: string; | ||
|
||
export let tooltip: (feature: MapGeoJSONFeature) => string; | ||
|
||
const originalOnClick = (e: CustomEvent<MapGeoJSONFeature>) => {}; | ||
|
||
export let onClick = originalOnClick; | ||
|
||
$: clickable = onClick !== originalOnClick; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
|
||
// The caller must also fill in the default slot with the contents of a help modal | ||
|
||
// @ts-ignore TODO Also constrain name to exist in the colors type | ||
let color = colors[name]; | ||
|
||
overwriteSource( | ||
$map, | ||
name, | ||
`${import.meta.env.VITE_RESOURCE_BASE}/layers/v1/${name}.geojson` | ||
); | ||
overwriteSource($map, name, url); | ||
|
||
overwriteCircleLayer($map, { | ||
id: name, | ||
|
@@ -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 commentThe 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 |
||
let name = feature.properties.name ?? `Unnamed ${singularNoun}`; | ||
return `<p>${name}</p>`; | ||
} | ||
</script> | ||
|
||
<Checkbox id={name} bind:checked={show}> | ||
|
@@ -53,4 +52,4 @@ | |
</span> | ||
</Checkbox> | ||
|
||
<InteractiveLayer layer={name} {tooltip} {show} clickable={false} /> | ||
<InteractiveLayer layer={name} {tooltip} {show} {clickable} on:click={onClick} /> |
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