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

conflict alerts display #27

Merged
merged 6 commits into from
Nov 13, 2024
Merged

Conversation

Tschonti
Copy link
Collaborator

Displays the conflicts around the world when that alert type is active

Things to note:

  • The json file that stores the ~5000 conflicts is almost 1MB, which takes some time to load, maybe we need a loading indicator
  • The result is cached locally, so it's faster on the second time, but still quite long as leaflet has to draw a bunch of markers (somehow this is way faster in the old map, might spend some time researching this in the next few days)
  • On max zoom, there is no clustering, I think this generally looks better. However, it's a bit weird around Israel:
    image
  • I didn't implement the legend, since the generic component is not ready yet

@Tschonti Tschonti requested a review from marinovl7 November 11, 2024 17:06
Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for wfp-hungermap ready!

Name Link
🔨 Latest commit 4091599
🔍 Latest deploy log https://app.netlify.com/sites/wfp-hungermap/deploys/6733ad44707b540008d5bba3
😎 Deploy Preview https://deploy-preview-27--wfp-hungermap.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@marinovl7 marinovl7 left a comment

Choose a reason for hiding this comment

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

Generally I'd say its a good job! However as you mentioned the rendering is expensive the first time and takes longer. We need to find a solution for that. Also please merge the new main status and add a legend so we have this completed, the legend component is already merged!

src/components/Map/ConflictLayer.tsx Outdated Show resolved Hide resolved
src/components/Map/ConflictLayer.tsx Outdated Show resolved Hide resolved
src/components/Map/ConflictLayer.tsx Outdated Show resolved Hide resolved
src/components/Map/ConflictLayer.tsx Outdated Show resolved Hide resolved
src/components/Map/Map.tsx Outdated Show resolved Hide resolved
src/components/Map/Map.tsx Outdated Show resolved Hide resolved
src/operations/ConflictOperations.ts Outdated Show resolved Hide resolved
src/operations/ConflictOperations.ts Outdated Show resolved Hide resolved
@marinovl7 marinovl7 self-requested a review November 12, 2024 18:04
Copy link
Collaborator

@marinovl7 marinovl7 left a comment

Choose a reason for hiding this comment

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

Overal good job! We need to find a solution for the optimizations! Can you please add legend to the alerts, since the legends PR is already merged so we can roll out this feature without the loading state only for now!

@Tschonti
Copy link
Collaborator Author

I added the legend. The description tooltip doesn't support markdown and doesn't look good with long text, so I just used the first paragraph from the current version.

Also implemented a very basic loading state: a spinner shows up next to the legend title. We can rollback this if you don't like it, but I think it's better than nothing.

The latency is weird, it doesn't seem to be leaflet that's slow, it's the sidebar context, but I have no idea why...

and unfortunately the zooming is quite ugly with the markers and mapbox, not sure what we can do with this...

@Tschonti Tschonti requested a review from marinovl7 November 12, 2024 19:37
Copy link
Collaborator

@marinovl7 marinovl7 left a comment

Choose a reason for hiding this comment

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

LGTM! Let's discuss the loading indicator + the zooming functionality and the sidebar context later in the meeting. It's not perfect yet, but its understandable tho, this is the first alert feature, so thank for providing that! Awesome job!

@marinovl7 marinovl7 merged commit 740d77a into main Nov 13, 2024
5 checks passed
@marinovl7 marinovl7 deleted the feature/f-45-conflict-alerts-display branch November 13, 2024 14:12
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.

2 participants