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

Missing map shapes in summary step #791

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

robinmolen
Copy link
Contributor

Part of open-formulieren/open-forms#5038

Because the geoJsonGeometry is available before the featureGroupRef is set, can the useEffect hook not draw the map shapes.

This fix makes sure that featureGroupRef is set, so that map shapes can be drawn

Copy link

codecov bot commented Jan 30, 2025

Bundle Report

Changes will increase total bundle size by 351 bytes (0.0%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
@open-formulieren/sdk-OpenForms-umd 4.78MB 179 bytes (0.0%) ⬆️
@open-formulieren/sdk-esm 4.75MB 172 bytes (0.0%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: @open-formulieren/sdk-esm

Assets Changed:

Asset Name Size Change Total Size Change (%)
assets/sdk-*.js -7 bytes 1.01MB -0.0%
assets/LeafletMap-*.js 179 bytes 366.32kB 0.05%

Files in assets/LeafletMap-*.js:

  • ./src/components/Map/types.jsx → Total Size: 503 bytes

  • ./src/components/Map/LeafletMap.jsx → Total Size: 7.35kB

view changes for bundle: @open-formulieren/sdk-OpenForms-umd

Assets Changed:

Asset Name Size Change Total Size Change (%)
open-*.js 179 bytes 3.49MB 0.01%

Files in open-*.js:

  • ./src/components/Map/LeafletMap.jsx → Total Size: 7.35kB

  • ./src/components/Map/types.jsx → Total Size: 503 bytes

Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.85%. Comparing base (8548829) to head (7d1521a).

Files with missing lines Patch % Lines
src/components/Map/LeafletMap.jsx 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #791      +/-   ##
==========================================
- Coverage   83.86%   83.85%   -0.01%     
==========================================
  Files         245      246       +1     
  Lines        4808     4814       +6     
  Branches     1279     1280       +1     
==========================================
+ Hits         4032     4037       +5     
- Misses        747      748       +1     
  Partials       29       29              
Flag Coverage Δ
storybook 77.45% <83.33%> (+<0.01%) ⬆️
vitest 62.61% <0.00%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

},
// `featureGroupRef.current` is needed as dependency to make sure that
// the featureGroupRef can be used.
[geoJsonGeometry, featureGroupRef.current]
Copy link
Member

Choose a reason for hiding this comment

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

Can you check these linter warnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the setup a bit to 'solve' this issue.

The problem was that the featureGroupRef was set after this useEffect was be called. Resulting in featureGroupRef.current not being defined/targetable. By adding the featureGroupRef.currect to the useEffect dependencies, featureGroupRef would trigger the useEffect once when it was defined.
But React doesn't like ref.currect as dependency, resulting in warnings/errors.

So i've changed the setup by creating a React component that handles the featureGroup updating. With this setup featureGroupRef.currect isn't needed as a dependency, because now featureGroupRef.current is set before we use it. This also separates the featureGroup updating functionality from the other map functionality.

Restructured the drawing logic of the map component, ensuring that geoJsonGeometry and the featureGroupRef are available. This also further encapsulates the drawing logic from the more general map functionalities.
@robinmolen robinmolen force-pushed the bug/5038-map-shapes-not-shown-in-summary-view branch from 8408f1e to 7d1521a Compare February 20, 2025 13:51
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.

Optional map, where no point is selected, gives an error.
2 participants