-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
✅ Deploy Preview for wfp-hungermap ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this 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!
There was a problem hiding this 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!
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... |
There was a problem hiding this 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!
Displays the conflicts around the world when that alert type is active
Things to note: