Map component single-interaction-mode rework#285
Conversation
…action" mode To prevent visual confusion, the interaction buttons in the map will always be shown. Even when the component is configured to only support one type of interaction, do we show the active interaction as clickable button. This was previously hidden to keep the UI simplistic (introduced in #5378). Manually clicking the interaction buttons, when only one interaction type was allowed, seemed like a hassle for users. This also broke consistency in regard to previous versions of the map component (where there was always only one possible type of interaction).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #285 +/- ##
==========================================
- Coverage 69.28% 69.26% -0.02%
==========================================
Files 287 288 +1
Lines 10796 10822 +26
Branches 1749 1800 +51
==========================================
+ Hits 7480 7496 +16
- Misses 3312 3322 +10
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Because we now always show the interaction buttons, we should trigger the "drawing mode" through the interaction buttons. Leaflet draw doesn't support this, so we have to make custom controls for this. The `overloadLeafletDrawToolbarControls` function adds controls to the Leaflet.Control.Draw to enable and disable drawing mode for a specific type of interaction. Controlling the drawing mode this way allows us to reuse the interaction buttons and their sub-menus (like "cancel", "finish shape", "remove last point", etc.).
We only enter drawing mode programmatically when the map component is empty. So when the user first opens the form or after deleting the previous map data (using the thrash button). This solves the problem where there would be multiple pins on the map; one for the current map data, and one for placing a new pin on the map. This caused confusion for users, as they weren't sure when they actually placed the pin and could continue the form.
The SingleInteraction storybook test uses `window._OF_INTERNAL_leafletMap` to place the pins. This is due to proj4 race-conditions when enabling/disabling the automatic drawing mode.
This could also have been done with `await userEvent.click(map, {x: 100, y: 100});`, but then we would receive "coordinates must be finite numbers" errors. These are thrown when we interact with the map before proj4 is ready. (sleep(200ms) would 'solve' this, but is rather flaky and risky)
966d8ae to
7d9b5a7
Compare
| @@ -0,0 +1,59 @@ | |||
| import * as Leaflet from 'leaflet'; | |||
There was a problem hiding this comment.
See 58a19d1 - could this be the nature of the race condition(s) you're experiencing?
There was a problem hiding this comment.
Mhh, no unfortunately not. The issue that I faced here seems to be an leaflet-draw internal "issue".
By clicking the map component using userEvent.click(map, {x: 100, y: 100}) leaflet-draw, internally using proj4, needs to figure out which coordinates correlate to the x,y point. The first interaction with userEvent.click is fine, but for the subsequent interactions the coordinates don't seems to be finite numbers (I suspect because the projection isn't loaded yet, so the point cannot correctly be calculated). After a few milliseconds proj4 can correctly interpret the coordinates and can calculate the positions.
So, we could add some sleep(200ms) calls in between, but that would unnecessarily slow-down the test. (And isn't guaranteed to work on all environments, as "proj4 getting ready" could take slightly longer)
By calculating the points using leafletMap.containerPointToLatLng() and firing the click event directly on the map instance, it seems we don't run into this issue. I think this uses a different part of the leaflet/proj4 code to calculate the points and do transformations, but I'm not entirely sure..
TL;DR: These race-condition don't seems to be caused by an import miss-match between Leaflet and Leaflet-draw, but due to some Leaflet-draw / proj4 internal stuff. I'll update the import statement, just in case any other unforeseen issues pop-up
Safety precaution related to review comment #285 (comment)
Partly closes open-formulieren/open-forms#5798, open-formulieren/open-forms#5819
The map component "single-interaction-mode" has been reworked to reduce confusion for users. Now we always show the interaction buttons, and only enter drawing mode when the user would want to enter the drawing mode.
To solve the issue of open-formulieren/open-forms#5819, and produce a more expected user experience, we now always show the interaction buttons. This allows the user to change the map data whenever they want, and keeps the UI consistent with the regular multiple interactions modes.
As described in open-formulieren/open-forms#5798 (comment), the "single-interaction-mode" will now only be placed in a "active drawing" state when the map is empty. To change the map data the user must either delete the current shape (using the 'trash can' button), or manually enter drawing mode by clicking the interaction buttons. This prevents scenario's where users would see multiple "pin" icons right after placing a pin on the map (the issue of open-formulieren/open-forms#5798).
Because we now always show the interaction buttons, we should use these for the automatically triggered "drawing mode". Leaflet-draw doesn't provide any tooling for this, so I've added some custom functions to
Leaflet.Control.Draw. What this all comes down to is: when we programmatically trigger "drawing mode" we also open the interaction buttons and show their sub-menus (the "cancel", "finish shape", "remove last point", etc. buttons). This allows us to provide full access to the Leaflet-draw interaction features, and keeps the UI/UX similar to when a user would themselves enter drawing mode.