feat(POI maps): add navigation controls for multi features popup

[RafaelSzmarowski]

Summary

The goal for this PR is to test a simple implementation to add navigation controls inside the popup when the click event query multiple features.

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

Screen.Recording.2024-01-11.at.11.39.50.mov

Native Popup close button has been removed to add a custom arrow which is at the same level of the navigation buttons.

We also update how a feature is highlight. Highlight and un-highlight strategies are different for each layer type. Currently, only 'symbol' and 'circle' has highlight and un-highlight strategies.

There is a small glitch when a symbol feature is selected: the layer flickers. I didn't find why, but I think this is acceptable.

Review checklist

• 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

[KevinFabre-ods]

Nice ✨

I don't see anything that could block this POC.

IMO We should also generate the close button in the same function that generate the arrow buttons

[KevinFabre-ods]

As popup is an optional configuration for a layer we must filter availableFeaturesOnClick with only features that has a popup config.

[RafaelSzmarowski]

Thanks for your feedback, indeed the close button should be part of the same row as the navigation controls, I think. And good catch for the this filter. I'll do the changes if everyone agrees for this first implementation 👍

[KevinFabre-ods]

Done here 7531261

[RafaelSzmarowski]

Another thing that I wasn't unsure and wanted to double check with you, is the change from setHtml to setDomContent safe enough, as far as I've read, there shouldn't be any change in terms of security. I mean both are XSS unsafe and should be handled by user to make sure the content passed to the popup is sanitize.

[KevinFabre-ods]

Another thing that I wasn't unsure and wanted to double check with you, is the change from setHtml to setDomContent safe enough, as far as I've read, there shouldn't be any change in terms of security. I mean both are XSS unsafe and should be handled by user to make sure the content passed to the popup is sanitize.

Yes, nothing changes, the value returned by getContent cannot be controlled.
It's up to the developer using the SDK component to be vigilant.

[etienneburdet]

I kinda think the popup can be its own Svelte component already, without much modifications to Map.ts actually. If updatePopupContent takes a callback, we can use that callback to update a let availableFeatures and then have some {#if availableFeatures} <Popup {availableFeatures} /> {/if} and then we can do everything inside the popup component I think.

That would be one less thing to migrate and unless I miss something, not as much work as it seems in the first place.

[KevinFabre-ods]

I kinda think the popup can be its own Svelte component already, without much modifications to Map.ts actually. If updatePopupContent takes a callback, we can use that callback to update a let availableFeatures and then have some {#if availableFeatures} <Popup {availableFeatures} /> {/if} and then we can do everything inside the popup component I think.

That would be one less thing to migrate and unless I miss something, not as much work as it seems in the first place.

@etienneburdet If a Svelte component controls the popup instance we must remove the 'popup' variable declaration in the Map.ts file.
This requires to move the updatePopupContent and updatePopupDisplay functions in the Popup component. Also when a popup is closed, we must update a map feature state to change how big a point is.

IMO all these changes required to completely rework on how the information is circulating between popup and map instance and should be done during our migration to Svelte components.

[KevinFabre-ods]

@RafaelSzmarowski I can't assign you as reviewer but can you please review it.
Since your work, I updated the code based on the comments we shared.

[RafaelSzmarowski]

@RafaelSzmarowski I can't assign you as reviewer but can you please review it. Since your work, I updated the code based on the comments we shared.

Thanks for the last changes and for taking care of this PR ! As far as I have tested it, it looks very well and the code LGTM 🏅 Nice work !

[KevinFabre-ods]

Seen with the design team to always show the navigation controls. So controls must be visible when the content is loading
Done in 4610afa

Screen.Recording.2024-01-16.at.16.54.25.mov

[etienneburdet]

All good for me if we migrate that to a Svelte component later 👌