[FEAT][EXPLO][SC-46279] Table - Geo field

[KevinFabre-ods]

Summary

The goal for this PR is to explore how we can implement the table geo field

Screen.Recording.2024-06-28.at.11.23.49.mov

(Internal for Opendatasoft only) Associated Shortcut ticket: sc-46279.

Changes

Did a lot of refactoring because my main idea was to re-use some part of the code that composes MapPoi. So I extracted a Svelte component (placed it in Map/WebGl) that can be used as a common template to generate maps. It is currently used by the PoiMap and Table blocks.

There should no visual and functional changes for the PoiMap visualization.

So, the table has a new column: GeoColumn with a lot of options:

• display: a function based on the cell value to render a text from complexe object (coordinates, shapes, etc...)
• mapOptions: an object where we configure render parameters for the map (like its style, center, bbox, interactiveness, etc...). In our application, this is where we pass the basemap style.
• layers: a function to render map layers based on the cell value.
• sources: a function to render map sources based on the call value

Currently, the WelGL map component only supports circle and symbol layers. If we go in this direction, we need to add support for the fill layers. To render geoshapes in our application. Done in #246

In this PR, we also add aliases for visualizations paths

• Description is complete
• Commits respect the Conventional Commits Specification
• 2 reviewers (1 if trivial)
• Tests coverage has improved
• Code is ready for a release on NPM

[etienneburdet]

Didn't fully review yet, but just a quick factor comment. Firts, the ChoroplethMap rename was long due… thanks! I don't think we need to extract the Map.svelte into a utils folder—they usually end up being a mixed bag of everything. As it is it's fine in PoiMap, GeoCell can totally import from there and it's not applicable as is for other maps.

The main factor we need now is to move to transformStyle which should simplify a lot of things, including a common MapRender.svelte, which could lead to something like this: 

Map
├── WegGl.svelte // can be imported in table
├── Svg.svelte
└── Choropleth
    ├── ChoroplethSvg.svelte // imports Svg
    ├── ChoroplethVectorTiles.svelte // imports WebGl
    ├── ChoroplethGeojson.svelte // imports WebGl
└── Poi
    ├── Poi.svelte // can be imported in Table // imports WebGl

That was pretty much the thinking at the time (hence the /Map/Choropleth folder) and I think it has quite a few advantages, especially in how we avoid doubling maplibre implementations.

[KevinFabre-ods]

Didn't fully review yet, but just a quick factor comment. Firts, the ChoroplethMap rename was long due… thanks! I don't think we need to extract the Map.svelte into a utils folder—they usually end up being a mixed bag of everything. As it is it's fine in PoiMap, GeoCell can totally import from there and it's not applicable as is for other maps.

The main factor we need now is to move to transformStyle which should simplify a lot of things, including a common MapRender.svelte, which could lead to something like this:

Map
├── WegGl.svelte // can be imported in table
├── Svg.svelte
└── Choropleth
    ├── ChoroplethSvg.svelte // imports Svg
    ├── ChoroplethVectorTiles.svelte // imports WebGl
    ├── ChoroplethGeojson.svelte // imports WebGl
└── Poi
    ├── Poi.svelte // can be imported in Table // imports WebGl

That was pretty much the thinking at the time (hence the /Map/Choropleth folder) and I think it has quite a few advantages, especially in how we avoid doubling maplibre implementations.

@etienneburdet I see where you want to go. Looks good. This is not light work to do. We might want to start with the current file organization to fulfill our table story scope by the end of the month.

After that, we can start with the refactoring.

Currently, we have a Map.ts file that is responsible for instantiating a Maplibre instance and defining POI behaviors (hover on feature, click on feature) and other more common behaviors like popup displays, reactive properties (center, bbox, zoom, layers, sources, interactivity).

How do you see the refactor for this file?

[etienneburdet]

So beside the fact I would put the components in Map rather than utils—which isn't critical either—, the grouping itself loosk good and works well 👍

The geoformat field works pretty well. UX-wise, I think I would be more for a modal/popin that centers in the middle of the table (or where we click). This way we don't risk breaking the other layouts. Could be for a next ticket though. I also have in mind the same kind of pattern for images: thumbnail that we can enlarge as a popup. Svelte transitions could definitely be of use here.

As for the Map.ts refactor, first and foremost, since we mainly don't touch the old code, but just break it appart, I think we can keep that for another PR. But my idea would be something like:

// Maplibre.svelte
<script>
setContext(map);
</script>

{#if map && maready}
  <slot />
{/if}

// Choropleth.svelte
<script>
export let options
const map = getContext(map);

onMount(() => initializeChoropleth(options);
</script>

<PopUp />
<Controls />

//ChoroplethVectorTiles
<Maplibre>
  <Choropleth {vectorTilesOptions} />
<Maplibre

The idea here would be that everything that requires enqueue is in the slot, so that it can be transformed into pure JS functions, that are called in initilialize. Everything that create HTML, is its own component (again guarded with the slot the guarentees loading). And then all reactive stuff (mainly layers) should belong to transformStyle if I'm correct.

[RafaelSzmarowski]

Nice work ! The GeoFormat cell is easy to read and understand even when discovering the code for the first time !

I agree with @etienneburdet to avoid the utils folder but it’s not critical and can be made later indeed. By the way, nice suggestion to use the slots, haven’t tested it but if it works it’s nice and clean.

I haven’t followed the UX discussions around the map display inside the table so I don’t know the final features of the table cell and I might be wrong:

IMO if the cell will be small and static like now, I automatically think about something else than a webGL (have you thought about a png or svg generated through map.getCanvas().toDataURL ? I don't know the impact in terms of performance...) but if the cell must be interactive then it should be a Maplibre webGL and probably in an expandable modal / popup as the current size is too small to have interactivity.

The webGL browser limit could be reached very quickly :

Enregistrement.de.l.ecran.2024-06-17.a.11.07.22.mov

I guess it could be dealt with by displaying only one map at a time in a bigger modal / popup or if we keep the current implementation, maybe by a better handling of webglcontextlost to avoid having broken cells like in my screen recording.

A basic listener could do the job but there might be a better solution:
in GeoFormat.svelte

    import { onMount, afterUpdate } from 'svelte';
    import Map from '../../../utils/Map';
    import type { MapOptions, MapData } from '../../../utils/Map/types';

    export let rawValue: unknown;
    export let display = (v: unknown) => v;
    export let mapOptions: MapOptions;
    export let sources: (v: unknown) => MapData['sources'] = () => ({});
    export let layers: (v: unknown) => MapData['layers'] = () => [];

    let showMap = false;
    let mapContainer: HTMLDivElement | null = null;

    $: data = {
        sources: sources(rawValue),
        layers: layers(rawValue),
    };

    function onLabelClick() {
        showMap = !showMap;
    }

    function handleWebGLContextLost(event: Event) {
        // Prevent the default behavior of the event
        event.preventDefault();
        // Close the map component properly
        showMap = false;
    }

    function setupWebGLContextListener() {
        if (mapContainer) {
            const canvas = mapContainer.querySelector('canvas');
            if (canvas) {
                canvas.addEventListener('webglcontextlost', handleWebGLContextLost, false);
            }
        }
    }

    onMount(() => {
        if (showMap) {
            setupWebGLContextListener();
        }
    });

    afterUpdate(() => {
        if (showMap) {
            setupWebGLContextListener();
        }
    });
</script>

<div>
    <!-- svelte-ignore a11y-click-events-have-key-events -->
    <div class="label" on:click={onLabelClick}>{display(rawValue)}</div>
    {#if showMap}
        <div class="table-cell-map-container" bind:this={mapContainer}>
            <Map options={mapOptions} {data} />
        </div>
    {/if}
</div>

<style>
    .label {
        cursor: pointer;
        text-decoration: underline;
    }
    .table-cell-map-container {
        width: 100%;
        height: 250px;
    }
</style>

[KevinFabre-ods]

Thank you @etienneburdet and @RafaelSzmarowski for your reviews

I'll update and move all files related to maps into the /Map folder.

As for the UX, I'm waiting on the design. I don't want to dive into an implementation that doesn't align with what the design team is planning.

Regarding the WebGL context loss, I'm also waiting for the design. Perhaps they will implement a feature where only one map can be displayed at a time.

About using other resources than Webgl to display a map, I considered it but didn't pursue it because:

• Attributions are not visible when an image is generated from map.getCanvas.
• We need to ensure that the layers have been loaded and are visible before calling map.getCanvas.
• IMO, if we want to use images instead of WebGL maps, the images should be generated server-side to avoid layers loading issues as described. With the image column type, you can display these images.

[etienneburdet]

I think map.getCanvas().toDataURL will have to pop a webgl instance anyways (before rexporting it to image), so it might not solve the problem if we don't save images somewhere. I'm very much in favor of "only one mini map at a time", both for UX and performance.

[RafaelSzmarowski]

I think map.getCanvas().toDataURL will have to pop a webgl instance anyways (before rexporting it to image), so it might not solve the problem if we don't save images somewhere. I'm very much in favor of "only one mini map at a time", both for UX and performance.

Yep, it doesn't seem like a solution and probably not better in performance, it was more to open discussion and check by curiosity if you have discussed other implementations already. I just thought that generating the image and removing the webGl would free the buffer and bypass the limit but beside this it's not a good idea and seems heavy and very limited in terms of interactions. So probably one map at a time is the best solution or just clean the webglcontextloss to avoid broken cells.

[KevinFabre-ods]

@etienneburdet & @RafaelSzmarowski

This PR is ready for a second round of review.
Since your last review:

• aliases have been added for visualizations package folders
• A minimap can be previewed (in a tooltip)when user focus or hover a geo cell

Changes in Chromatic, is due to the addition of a new column

[RafaelSzmarowski]

Nice work ! As far as I have tested it, it looks clean and nice, way better UX in my opinion than displaying the image inside the cell.

I have left only non blocking minor comments.

[etienneburdet]

Working well on my side! I just need to look into this TS error before I validate.

I assume the idea is to move Choroplethmap back into the new Map folder and using the WebGL.svelte for both. I'm fine with, we'll just have to be careful with TS alsiases when importing in the meantime.

And I also assume we make another story/PR for the modal (which seems a good idea indeed).

[KevinFabre-ods]

Thanks for your review.

Indeed, there were TypeScript errors in the GeoFormat.svelte component. Sorry about that. They should be fixed now. However, the CI should have caught these errors.

In my opinion, we were missing an ESLint check for the Svelte components. This has been fixed in this commit. I checked that these TypeScript errors were caught by the new test.

As we discussed this morning, we have an issue when building the Svelte package. The introduction of aliases breaks the paths for the TypeScript declaration files. @RafaelSzmarowski, this morning, with @etienneburdet, we tested various Rollup plugins to fix it. We were unsuccessful. So, we have decided to try upgrading Rollup to the latest version if it is quick to do.

If not, we will revert the alias commit f24f939ad6a953924473c5e72d9182cde9428593 to avoid blocking this PR and to address the Rollup update in a dedicated PR.

We will sum up tomorrow morning to check the progress of the upgrade and decide what to do in this PR.

[etienneburdet]

All good for me! I like the new classes better indeed too.

Svelte check should do, but I'm surprised it didn't work, we used to have implicit any errors on uninitialized variables. But it works… 🤷‍♂️

[cybervinvin]

I arrive after the battle but, in any case, I don't have much to say on a subject that I don't really understand.
Now, overall the code seems easier to read and that's generally a good sign.
However, I have 2 questions on very small points of detail which are absolutely not blocking.

Bravo and thanks for the work 👏🏼

[cybervinvin]

It seems that this import is useless

[KevinFabre-ods]

🤔 Why ?
It is used at line 6

ods-dataviz-sdk/packages/visualizations/src/components/Map/Poi/types.ts

Line 6 in 9ae11aa

export type PoiMapData = Async<WebGlMapData>;

[cybervinvin]

i don't understand why many atributes here are !important

[KevinFabre-ods]

I was able to remove most of the !important style expect for the transform as it is set directly on the Node right after the popup opens

https://github.com/maplibre/maplibre-gl-js/blob/b63b28454a7c43341d41211c0e9c63948597e8ee/src/ui/popup.ts#L647