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

refactor(map): TypeScriptize Project Data Map files DEV-16 #5558

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

Conversation

magicznyleszek
Copy link
Member

@magicznyleszek magicznyleszek commented Feb 27, 2025

🗒️ Checklist

  1. run linter locally
  2. update all related docs (API, README, inline, etc.), if any
  3. draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
  4. tag PR: at least frontend or backend unless it's global
  5. fill in the template below and delete template comments
  6. review thyself: read the diff and repro the preview as written
  7. open PR & confirm that CI passes
  8. request reviewers, if needed
  9. delete this section before merging

💭 Notes

Changes here:

  • Improved map related actions definitions in actions.d.ts
  • Renamed map related files
  • MapSettings
    • Split out MapColorPicker component out of MapSettings file
    • Map settings tabs use enum now instead of string
    • Dropped autoBind and listener mixins
    • Split one translation into two (acted on TODO comment)
  • FormMap
    • Is now index.tsx file
    • Standardized some error handling in the file (there is still room for improvement, but there is high chance we would be rewriting this whole route 🤷, handling errors was useful for me debugging things, so I left the code there and polished a bit)
    • Improved the flow of handling asset files fetch response
      • removeUnknownLayers function is doing a cleanup
      • addNewLayers is ensuring all different file types end up using the same onOmnivoreLayerReady callback instead of relying on things "just working"
      • GeoJSON is being validated before being rendered (avoids code crashing)
    • Dropped state.loading as it wasn't being used for anything really
  • Added @placemarkio/check-geojson for validating GeoJSON (omnivore sadly doesn't do it, resulting in crashing UI when invalid GeoJSON is added)
  • Created some simple types for @mapbox/leaflet-omnivore

👀 Preview steps

Ideally test this branch with bunch of layer files (map test files.zip)

  • csv - Dentists in Calderdale April 2023.csv
  • geojson - Data Mill North - Leeds litter bin locations 20211201.json
  • geojson - Pedestrian Crossings.json
  • geojson (invalid) - Natural Earth’s vector data, version 4.1.0 land-110m.json
  • kml - lava field extend from the 2023-2024 svartsengi eruption 20231218.kml
  • kmz - earths-tectonic-plates-earthref.kmz
  • kmz - Smithsonian_volcanoes.kmz
  • wkt - Pedestrain Crossings converted_2025-02-26T14_32_34.330Z.wkt
  • wkt (invalid) - sample roads in Asia.wkt

It's important to test each type, as each type is being handled a bit differently by the code (especially geojson and kmz).

Testing an overlay file:

  1. ℹ️ have account and a project with geopoint question, text question and bunch of submissions
  2. go to project -> Data -> Map
  3. 🟢 notice that the map displays correctly
  4. open "Map display settings"
  5. type in some name and upload one of the layer files
  6. 🟢 notice that the layer contents should appear on the map (if you used a valid file)
  7. 🟢 notice that an error toast notification appears (if you used an invalid file)

After making above steps, ensure everything works as previously:

  1. "Toggle Fulscreen" should work (top right corner of map)
  2. "Show as points" should work (top right corner of map)
  3. "Show as heatmap" should work (top right corner of map)
  4. "Toggle layers" should work (top right corner of map) (both switching base map and toggling custom layers)
  5. "Disaggregate by survey responses" should work (bottom left corner of map)
    1. after you use it, you can verify that "Map display settings" -> "Marker colors" tab settings work correctly

…ta-map

# Conflicts:
#	jsapp/js/actions.d.ts
#	jsapp/js/components/map/map.es6
#	jsapp/js/components/map/map.js
#	jsapp/js/components/map/map.tsx
#	jsapp/js/components/map/mapSettings.es6
#	jsapp/js/components/map/mapSettings.js
#	jsapp/js/components/map/mapSettings.tsx
# Conflicts:
#	jsapp/js/actions.d.ts
#	jsapp/js/components/map/map.js
#	jsapp/js/components/map/mapSettings.tsx
#	jsapp/js/dataInterface.ts
@magicznyleszek magicznyleszek marked this pull request as ready for review February 27, 2025 13:18
@Akuukis Akuukis self-requested a review March 3, 2025 11:56
@Akuukis
Copy link
Contributor

Akuukis commented Mar 3, 2025

Found an issue

  • after uploading an invalid file, the toast appears all good, but the file still appears in the list without any indication that it's invalid. Furthermore, every time from now on when uploading another valid file the toast reappears.
    image

@magicznyleszek
Copy link
Member Author

Found an issue

* after uploading an invalid file, the toast appears all good, but the file still appears in the list without any indication that it's invalid. Furthermore, every time from now on when uploading another valid file the toast reappears.

I see, I will have to refactor the code a bit. I will move the notification to happen once immediately after upload, and will display error next to the item in the list rather than displaying notification each time. Does this sound good? :)

@Akuukis
Copy link
Contributor

Akuukis commented Mar 3, 2025

@magicznyleszek sounds to me 🆗 perhaps check with Tessa also?

Copy link
Contributor

@Akuukis Akuukis left a comment

Choose a reason for hiding this comment

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

see above

@magicznyleszek magicznyleszek mentioned this pull request Mar 3, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants