Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding map interaction config #198

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

robinmolen
Copy link
Contributor

@robinmolen robinmolen commented Dec 11, 2024

Part of open-formulieren/open-forms#2177

This relies on a update to the types repo: open-formulieren/types#62

@robinmolen robinmolen force-pushed the feature/2177-map-interactions branch 2 times, most recently from 1644d74 to e7254fd Compare December 16, 2024 08:53
@robinmolen robinmolen marked this pull request as ready for review December 16, 2024 08:54
@robinmolen robinmolen requested a review from vaszig December 16, 2024 08:55
@robinmolen robinmolen marked this pull request as draft December 16, 2024 10:55
@robinmolen
Copy link
Contributor Author

Because this feature will be added after 3.0, this PR is moved to draft.

This PR is still ready to be reviewed, it just shouldn't be merged just yet

Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can say, it seems good to me. I would add though some stories for the interactions (not necessarily tests, but just to have the appearance).

@robinmolen
Copy link
Contributor Author

As far as I can say, it seems good to me. I would add though some stories for the interactions (not necessarily tests, but just to have the appearance).

The map component stories do show the configuration, but the choice doesn't have a visual representation.
But yeah, it would probably be nice to show/test the interaction options in the builder..

@robinmolen robinmolen force-pushed the feature/2177-map-interactions branch 2 times, most recently from 56ca3aa to 72b3cc2 Compare December 18, 2024 14:56
@robinmolen robinmolen marked this pull request as ready for review January 6, 2025 10:40
@robinmolen
Copy link
Contributor Author

This PR can be reviewed (this includes some chromatic changes that also have to be reviewed)

In the formio-builder a new map component configuration option was added: interactions. "Interactions" is a group of checkboxes, allowing different user interactions in the map. To keep the logic close to what we already have, the pointer interaction is activated by default.

In the form builder map component preview you can test/use the active interactions.

@robinmolen robinmolen force-pushed the feature/2177-map-interactions branch from 72b3cc2 to ff16d26 Compare January 6, 2025 15:22
@robinmolen robinmolen force-pushed the feature/2177-map-interactions branch from ff16d26 to d6949a6 Compare January 7, 2025 10:55
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move all of the map settings to a separate tab? Tab title can be "Map settings" or just "Settings", and would come after the "Validation" tab. I think the "Basic" tab is getting too clutter.

Specifically, this means the fields:

  • Use globally configured map component settings
  • Map configuration (and re-label this to "Initial focus" or something like that)
  • Tile layer
  • Available interactions

@@ -151,6 +151,11 @@
"isTranslated": true,
"originalDefault": "CSS class"
},
"3xnsUT": {
"defaultMessage": "Lijn interacties",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"defaultMessage": "Lijn interacties",
"defaultMessage": "Lijn",

I feel like "interactions" is repeated way too much. I'm also inclined to rephrase this as "available drawing shapes" to make it simpler?

@@ -572,6 +577,11 @@
"description": "Label for 'disableAddingRemovingRows' builder field",
"originalDefault": "Disable adding or removing groups"
},
"NbDr3m": {
"defaultMessage": "Geeft gebruikers de mogelijkheid een lijn te tekenen, wanneer ze met het kaartmateriaal werken",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"defaultMessage": "Geeft gebruikers de mogelijkheid een lijn te tekenen, wanneer ze met het kaartmateriaal werken",
"defaultMessage": "Gebruikers kunnen rechte lijnen tekenen op de kaart.",

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it one or more lines, or just one? Probably good to clarify that in the tooltip.

@@ -598,6 +608,11 @@
"isTranslated": true,
"originalDefault": "Label"
},
"POkaFQ": {
"defaultMessage": "Polygoon interacties",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"defaultMessage": "Polygoon interacties",
"defaultMessage": "Veelhoek (polygoon)",

@@ -885,6 +900,11 @@
"description": "Tooltip for 'includePartners' builder field",
"originalDefault": "Whether to add partners information to the component."
},
"ZRM6J8": {
"defaultMessage": "Mogelijke kaartmateriaal interacties",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"defaultMessage": "Mogelijke kaartmateriaal interacties",
"defaultMessage": "Mogelijke kaartmateriaalinteracties",

single samengesteld woord

@@ -1158,6 +1178,11 @@
"description": "Label for 'ClearOnHide' builder field",
"originalDefault": "Clear on hide"
},
"iyWQAt": {
"defaultMessage": "Geeft gebruikers de mogelijkheid een polygoon te tekenen, wanneer ze met het kaartmateriaal werken",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"defaultMessage": "Geeft gebruikers de mogelijkheid een polygoon te tekenen, wanneer ze met het kaartmateriaal werken",
"defaultMessage": "Gebruikers kunnen een veelhoek (gesloten vorm die uit rechte lijnen bestaat) tekenen.",

@@ -1199,6 +1224,11 @@
"description": "Label for translation message for validation error code",
"originalDefault": "Error message"
},
"kIX7bm": {
"defaultMessage": "Pin interacties",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"defaultMessage": "Pin interacties",
"defaultMessage": "Pin/punt",

@@ -1464,6 +1494,11 @@
"description": "Tooltip for 'hideHeader' builder field",
"originalDefault": "Do not display the configured label and top line as the header in the fieldset."
},
"vFydxX": {
"defaultMessage": "Geeft gebruikers de mogelijkheid een pin te plaatsen, wanneer ze met het kaartmateriaal werken",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"defaultMessage": "Geeft gebruikers de mogelijkheid een pin te plaatsen, wanneer ze met het kaartmateriaal werken",
"defaultMessage": "Gebruikers kunnen een punt aanduiden op de kaart.",

Comment on lines 1 to 2
@import 'leaflet-draw/dist/leaflet.draw.css';
@import 'leaflet/dist/leaflet.css';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are these imports in an unrelated scss file? Can't you just import them close to where they're used (https://github.com/open-formulieren/formio-builder/blob/main/src/registry/map/index.ts)? And preferably a preview.scss file that loads these libraries, so we have a consistent pattern of loading stylesheets for a given context:

import './previews.scss';

Comment on lines 100 to 104
edit={{
edit: false,
remove: false,
}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do these control? Does it mean once I've drawn something, I can't remove it anymore from the map?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah they add delete and edit buttons to the map. They are a bit cumbersome when using, so i opted to remove them..
At the moment only 1 shape is allowed. So when you create a new one, the old shape is removed. This does mean that once you have added a shape, you cannot clear it..

Im taking a look if we can change the behavior of these buttons (i.e. remove all shapes when you click the button, instead of having to click to shape and then save the deletion). For reference, this is how it normally looks https://leaflet.github.io/Leaflet.draw/docs/examples/full.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah not being able to make corrections is a problem. I think it's probably best to enable the delete, but leave the edit disabled and I'm sure there will be accessibility feedback about this in the future.

@@ -18,6 +18,7 @@
"strictNullChecks": true,
"allowSyntheticDefaultImports": true,
"noErrorTruncation": true,
"skipLibCheck": true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the impact of this? I don't like this part from the documentation:

...at the expense of type-system accuracy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This skips type checking of *.d.ts files. With this, only the *.d.ts files that we directly use are type checked.

This is a 'solution' for a typescript error originating from node_modules/react-leaflet-draw/src/index.d.ts.

As we discussed in the office, maybe better to isolate this, or to solve this another way. I've gone trough the repo/issues of react-leaflet-draw, but i haven't found anything related to this.. So i created a ticket, asking their advice about using typescript alex3165/react-leaflet-draw#188

Both changes are needed for the leaflet-draw and react-leaflet-draw dependencies.

leaflet-draw uses images for the different markers, which are included in the css. For this to work with the scss build, .png and .svg files use the dataurl loader https://esbuild.github.io/content-types/#data-url

Typescript errors in the react-leaflet-draw dependency caused a typescript validation/error, which shouldn't happen. Using `skipLibCheck` in the tsconfig.json type issues in the node_modules folder are ignored
@robinmolen robinmolen force-pushed the feature/2177-map-interactions branch from d6949a6 to d27c01b Compare January 8, 2025 17:08
@robinmolen robinmolen force-pushed the feature/2177-map-interactions branch from d27c01b to faf26ff Compare January 9, 2025 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kaartmateriaal - Verschillende types kaart-interactie (Point, Polygon en Line)
3 participants