Boundary changes follow up frontend

[awdem]

This PR makes some tweaks to the boundary change maps. It:

• Simplifies the line interpolation. The minimum line width is now 3 to distinguish it from highways on OSM and it there are less unneccesary steps
• Adds popups on click for the old and new layers, as well as for the postcode centroid.
• changes the cursor to a pointer when passing over interactive layers

[coveralls]

coverage: 59.49% (+0.1%) from 59.365% — boundary-changes-follow-up-frontend into master

[GeoWill]

I haven't run this locally, but I found the screencast extremely useful! I would mostly be checking that the popups open and close as I would expect, and it works nicely on constituencies and regions (i.e. there's no accidental sharing of feature data between the maps or some such).

There's a few comments on the code.

Regarding showing the postcode in the popup I think that's a nice to have rather than needed.

[GeoWill]

I'm assuming this is done to make the popups work I.e. we need a transparent fill.
If this is the case, I don't think this needs to be a conditional. It looks like ShowNewBoundaryFill is set to true, then not used, then set to false and evaluated here. Shouldn't we just bin it, and set opacity to 0?
If I've missed something and we do need ShowNewBoundaryFill, can it be camel case: showNewBoundaryFill.

[awdem]

This is so the fill appears when when we're only showing the new boundaries (e.g. no change, or all Welsh postcodes), but is transparent when the old boundaries are also on the map.

[GeoWill]

Got it.
In that case I think it might actually be clearer to make this:

"fill-opacity": !showOldBoundary ? 0.2 : 0

[awdem]

That makes more sense. Have changed it in 0b33dac

[GeoWill]

startsWith returns a bool, so I don't think you need the ternary (? true : false)

[awdem]

You're right. Fixed in 6a58dd9

[GeoWill]

I know we control the pmtiles, but I think it's probably better here not to just chuck the colour and name strings into the html without sanitizing it. This is more so that we don't go and copy and paste it somewhere else when we shouldn't, rather than it being a massive concern here. It's also confusing because there's a js built in setHTML which does sanitize input, while maplibre popup setHTML doesn't...

I think if you build the popup up out of dom elements, and then use setDOMContent, it will avoid the issue.

const popupContent = document.createElement('div');
featsData.forEach(feat => {
    const featContainer = document.createElement('div');
    featContainer.style.color = feat.layerColor;

    const label = document.createTextNode(feat.isNew ? "New: " : "Old: ");
    const strong = document.createElement('strong');
    strong.style.color = feat.layerColor;
    strong.textContent = feat.name;

    featContainer.appendChild(label);
    featContainer.appendChild(strong);
    popupContent.appendChild(row);
});

const popup = new maplibregl.Popup({ closeButton: true, closeOnClick: true })
    .setLngLat(coords)
    .setDOMContent(popupContent);
showPopup(popup);

[awdem]

Ah I had not thought about that. I've implemented the above in c8f59a7.

[GeoWill]

You don't use featId any more now.

[awdem]

Whoops, fixed in ba5f123

[GeoWill]

I assume the actual postcode is available in the context, so I'd lean to including it here rather than just poscode.

[awdem]

Done in 7e30807

[awdem]

I would mostly be checking that the popups open and close as I would expect, and it works nicely on constituencies and regions (i.e. there's no accidental sharing of feature data between the maps or some such).

I just double-checked a few scottish postcodes and didn't find any accidental interactions between maps. popups acting normal as well.

[GeoWill]

I've pushed ba5f123 to development to trigger a deploy. I'll have a quick play around, but I think this looks good.

[GeoWill]

One more small issue. It's a bit annoying that the popup initialises with the close button highlighted, as it obscures the last letter of the postcode. I'm not sure why it's happening, I guess and easy fix would be to add a bit more padding?

[awdem]

One more small issue. It's a bit annoying that the popup initialises with the close button highlighted, as it obscures the last letter of the postcode. I'm not sure why it's happening, I guess and easy fix would be to add a bit more padding?

Turning this Mablibre popup option to False stops the button from being focused: https://maplibre.org/maplibre-gl-js/docs/API/type-aliases/PopupOptions/#focusafteropen

But I don't if that has any accessibility implications? Happy to change the padding but wanted to see what you think.

[GeoWill]

I'm not sure I'm afraid.
It does take a lot of tabbing to get round the whole page though. So maybe best to just pad it?

[awdem]

I'm not sure I'm afraid. It does take a lot of tabbing to get round the whole page though. So maybe best to just pad it?

Done in ac35c9a

[GeoWill]

I reckon we're good to apply the fixups and deploy 👍