nptscot / nptscot.github.io

Network Planning Tool for Scotland: front end.
https://www.npt.scot
GNU Affero General Public License v3.0
7 stars 5 forks source link

Add new UI element to show/hide cohesive network layer #130

Closed Robinlovelace closed 6 months ago

Robinlovelace commented 8 months ago

We need to be able to show the 'cohesive network', representing the core network.

The data will change but the structure will be GeoJSON. As promised, here's an example dataset: https://github.com/nptscot/coherentnet/releases/download/v0.0.1/rnet_subset.geojson

mvl22 commented 8 months ago

Great to see this. Is this going to be converted to vector tiles as per our earlier call, or am I going to need to write new GeoJSON handling into the codebase?

Robinlovelace commented 8 months ago

Great to see this. Is this going to be converted to vector tiles as per our earlier call, or am I going to need to write new GeoJSON handling into the codebase?

It will be converted to a .pmtiles file, just wanted to get you the raw data asap.

mvl22 commented 8 months ago

Could you give some guidance as to how this should be presented, i.e. whether this is part of the 'Route network' family of functionality, or 'Other layers'? The latter will be much quicker to get going but I'm not sure you want it there.

Robinlovelace commented 8 months ago

Could you give some guidance as to how this should be presented, i.e. whether this is part of the 'Route network' family of functionality, or 'Other layers'? The latter will be much quicker to get going but I'm not sure you want it there.

First thought would be part of the route network family but not sure and I think either could work. We may want to get feedback from @anguscalder or Matt on this. I suggest you put it in 'Other layers' as a starter for 10 and go from there.

Robinlovelace commented 8 months ago

Update on this FYI @mvl22, in conversation with Angus this seems a good place, directly to the right of the simplified network toggle:

image

mvl22 commented 8 months ago

Let me know when this is on the server as PMTiles and I'll get this into the UI.

Robinlovelace commented 8 months ago

Let me know when this is on the server as PMTiles and I'll get this into the UI.

Here's an example PMTiles dataset, will work fine on localhost for testing: https://github.com/nptscot/npt/releases/download/cohesive-v1/example_cohesive.pmtiles

Robinlovelace commented 8 months ago

At present: there's only one attribute key: group. That can be the basis of the slider. Here's how it looks, in the spirit of a minimal example:

image

mvl22 commented 8 months ago

Screenshot 2024-01-31 at 14 25 31

Robinlovelace commented 8 months ago

Looks good and works well in the Other Layers section I think. Heads-up @anguscalder.

Next logical steps I think are:

Robinlovelace commented 8 months ago

Just to check @mvl22 should they be called cohesivenetwork.pmtiles at the same URL path as the others, and where should they go locally? I'm looking at line 19 of layer_control.js which currently has this:

    ['cohesivenetwork', {localUrl: 'cohesivenetwork/'}],

I imagine when it's live that can change to

    ['cohesivenetwork'],

but want to check my assumptions.

Robinlovelace commented 8 months ago

Now on test website:

image

mvl22 commented 8 months ago

Just to check @mvl22 should they be called cohesivenetwork.pmtiles at the same URL path as the others,

That's right - same pattern as the others.

, and where should they go locally?

['cohesivenetwork', {localUrl: 'cohesivenetwork/'}],

Yes, that localUrl is just the path from root of the site if doing local testing. If you save it somewhere else, e.g. myfolder/cohesivenetwork/ then that would be the value to use. Maybe we should define a standard location for putting these in when testing.

I'll make this stuff clearer as the layer stuff is simplified further (partially blocked on https://github.com/nptscot/npt/issues/385).

mvl22 commented 8 months ago

Get direction on the UX (my hopothesis: we want to add a slider to allow the network to grow) from @Mattpdavis and Angus

Yes, I've not tried to do anything more yet, and it probably does need it. The layers section needs more refactoring before we can more easily add sliders/filters. My priority was just to get it online at this stage so you have confidence in viewing the data.

Can you explain what the numbers 1-4 represent? Will be able to put in labels trivially now, in the HTML, as per the other sliders:

https://github.com/nptscot/nptscot.github.io/blob/refactor/index.html#L242-L243

mvl22 commented 8 months ago

Just to flag up as you commit your code to generate the file at some point, that the layer name is currently example_cohesive (matching the filename you sent), but this needs to become cohesivenetwork.

https://github.com/nptscot/nptscot.github.io/blob/refactor/js/layer_control.js#L93

Let me know when that is fixed up, or indeed just update the js/layer_control.js file as above directly.

Robinlovelace commented 8 months ago

Maybe we should define a standard location for putting these in when testing.

Agreed. How about static/ or local/?

Robinlovelace commented 8 months ago

Let me know when that is fixed up, or indeed just update the js/layer_control.js file as above directly.

Noted, thanks. cohesivenetwork it can be (although cohesive_network would be a bit better).

mvl22 commented 8 months ago

If you're happy with underscores in URLs, that's fine.

Robinlovelace commented 8 months ago

What's most widely accepted best practice in web development these days?

mvl22 commented 8 months ago

It's preference. But for me, a URL like

npt.scot/cohesivenetwork

is nicer than

npt.scot/cohesive_network

(For when I move onto layer state permalinking support, subject to time.)

Robinlovelace commented 8 months ago

Yeah happy with that. cohesivenetwork it shall be.

mvl22 commented 7 months ago

should they be called cohesivenetwork.pmtiles at the same URL path as the others, and where should they go locally?

Just to say that this is now specified differently, following various refactoring.

The layer definition itself is now in the definitions file at: https://github.com/nptscot/nptscot.github.io/blob/refactor/src/datasets.js#L125-L144 and you can see it now has a clear URL, also containing a placeholder for the tileserver URL to avoid that having to be restated.

Then, if you want to override this while testing locally, just add such an override in the settings file: https://github.com/nptscot/nptscot.github.io/blob/refactor/src/settings.js#L40-L43 where you can see you can specify the layer name, and the path to it from site root.

I hope this is now much clearer.

mvl22 commented 7 months ago

Get direction on the UX (my hopothesis: we want to add a slider to allow the network to grow)

I don't think I've heard further on this, but am happy to propose changes if you can explain exactly what this data is and what is meant by 'allow the network to grow'.

(If in practice this is a future thing that doesn't yet exist in data, I recommend avoiding speculative generality in the UI code for now, and tackling this when it does.)

Robinlovelace commented 7 months ago

I think it will make sense to have a quick meeting with me, @wangzhao0217 and possibly @anguscalder after we have a larger example.

mvl22 commented 7 months ago

Can you explain what the ‘group’ numbers 1-4 represent?

Robinlovelace commented 7 months ago

Can you explain what the ‘group’ numbers 1-4 represent?

Those are roughly the order in which the network will grow. We'll have some new data for you soon Martin. Cc @wangzhao0217

mvl22 commented 7 months ago

Ah that makes sense now, great.

Yes, let me know when it's on the server, and at what URL.

mvl22 commented 7 months ago

I suggest be a new main button in the sidebar, as it's essentially a separate dataset which will need its own slider. Let me know if this seems OK and I will start getting this in place.

Robinlovelace commented 7 months ago

Main button in sidebar sounds good to me.

Robinlovelace commented 7 months ago

Update on this, we're almost there with the new data @mvl22 should have something for you tomorrow, taster here: https://github.com/nptscot/npt/pull/411#issuecomment-1979475605

One question: how do we deal with different pmtiles? We need to build them as separate areas, should we combine them for the main website? I guess so...

mvl22 commented 6 months ago

Yes, each layer is a single layer, so data from individual areas needs to be combined.

mvl22 commented 6 months ago

The cohesive network is now promoted to its own top-level control.

I'm not convinced it makes sense to have a slider for this layer.

Screenshot 2024-03-06 at 21 21 53

Robinlovelace commented 6 months ago

Looking good! Cc @anguscalder and @Mattpdavis

mvl22 commented 6 months ago

I think this is all now in place - can this be closed?

Robinlovelace commented 6 months ago

First part closed. How hard would it be to create a slider that incrementally shows each of the (currently 12) groups @mvl22 ?

mvl22 commented 6 months ago

Not hard, I was asking though if it made sense from a user perspective re purpose. Sounds like you think it does then.

Robinlovelace commented 6 months ago

Not hard, I was asking though if it made sense from a user perspective re purpose. Sounds like you think it does then.

Makes sense to me but more importantly asked for by @anguscalder.