Skip to content
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

Merged
merged 6 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions assets/crossings.geojson
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this now

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions src/lib/browse/LayerControls.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import CensusOutputAreaLayerControl from "./layers/CensusOutputAreaLayerControl.svelte";
import CombinedAuthoritiesLayerControl from "./layers/CombinedAuthoritiesLayerControl.svelte";
import CriticalIssuesLayerControl from "./layers/CriticalIssuesLayerControl.svelte";
import CrossingsLayerControl from "./layers/CrossingsLayerControl.svelte";
import CycleParkingLayerControl from "./layers/CycleParkingLayerControl.svelte";
import CyclePathsLayerControl from "./layers/CyclePathsLayerControl.svelte";
import HospitalsLayerControl from "./layers/HospitalsLayerControl.svelte";
Expand Down Expand Up @@ -52,6 +53,7 @@
<MrnLayerControl />
<BusRoutesLayerControl />
<CycleParkingLayerControl />
<CrossingsLayerControl />
</CheckboxGroup>
</CollapsibleCard>
<CollapsibleCard label="Boundaries">
Expand Down
1 change: 1 addition & 0 deletions src/lib/browse/colors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export const colors = {
bus_route_with_lane: "#9362BA",
bus_route_without_lane: "#C2A6D8",
cycle_parking: "black",
crossings: "green",

// Thanks to https://github.com/cyipt/cyipt-website/issues/23
cycle_paths: {
Expand Down
64 changes: 64 additions & 0 deletions src/lib/browse/layers/CrossingsLayerControl.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<script lang="ts">
import { circleRadius } from "colors";
Copy link
Contributor

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

import { ExternalLink } from "lib/common";
import type { MapGeoJSONFeature } from "maplibre-gl";
import crossingsUrl from "../../../../assets/crossings.geojson?url";
Copy link
Contributor

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

import PointAmenityLayerControl from "./PointAmenityLayerControl.svelte";

const name = "crossings";

function tooltip(feature: MapGeoJSONFeature): string {
const crossingTypeString: string = feature.properties["crossing"];
Copy link
Contributor

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

if (crossingTypeString === "no")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does npm run fmt keep this? Lack of braces for an if are weird

Copy link
Contributor

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

return "Location where crossing is impossible/illegal but where there is a clear desire line to cross.";
let result = "Crossing with the following features.";
Copy link
Contributor

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

const initialTooltipSentence =
initialTooltipSentences.get(crossingTypeString);
if (initialTooltipSentence) result = initialTooltipSentence;

return result + `. More information can be found by clicking.`;
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

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

}

const initialTooltipSentences: Map<string, string> = new Map<string, string>([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general in TS, I don't think you need to do let x: Type = new Foo<Generic>(... -- probably just the first is sufficient.

["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",
Copy link
Contributor

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" ?

],
]);

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>
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>
31 changes: 18 additions & 13 deletions src/lib/browse/layers/PointAmenityLayerControl.svelte
Copy link
Contributor

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.

Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

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

Copy link
Contributor

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.


// 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,
Expand All @@ -38,11 +42,6 @@
// TODO Outline?
});
let show = false;

function tooltip(feature: MapGeoJSONFeature): string {
Copy link
Contributor Author

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

let name = feature.properties.name ?? `Unnamed ${singularNoun}`;
return `<p>${name}</p>`;
}
</script>

<Checkbox id={name} bind:checked={show}>
Expand All @@ -53,4 +52,10 @@
</span>
</Checkbox>

<InteractiveLayer layer={name} {tooltip} {show} clickable={false} />
<InteractiveLayer
layer={name}
{tooltip}
{show}
{clickable}
on:click={onClick}
/>
10 changes: 9 additions & 1 deletion src/lib/browse/layers/RailwayStationsLayerControl.svelte
Original file line number Diff line number Diff line change
@@ -1,16 +1,24 @@
<script lang="ts">
import { circleRadius } from "colors";
import { ExternalLink } from "lib/common";
import type { MapGeoJSONFeature } from "maplibre-gl";
import PointAmenityLayerControl from "./PointAmenityLayerControl.svelte";

const name = "railway_stations";
const url = `${import.meta.env.VITE_RESOURCE_BASE}/layers/v1/${name}.geojson`;

function tooltip(feature: MapGeoJSONFeature): string {
let name = feature.properties.name ?? "Unnamed railway station";
return `<p>${name}</p>`;
}
</script>

<PointAmenityLayerControl
{name}
singularNoun="railway station"
pluralNoun="Railway stations"
{circleRadius}
{url}
{tooltip}
>
<p>
This shows <ExternalLink
Expand Down
1 change: 1 addition & 0 deletions src/lib/maplibre/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@ const layerZorder = [
"bus_routes",
"railway_stations",
"cycle_parking",
"crossings",
"cycle_paths",
"vehicle_counts",

Expand Down
Loading