feat: Countries with very high level of hunger alert #17
Conversation
ahmedfarouk2000
commented
Nov 8, 2024
ahmedfarouk2000
commented
- There is some issues that might effect this hunger alert, this includes the request which takes a lot of time (avergae of 7 second or more) and sometimes it timed out. so I dont know if the backend will be refactored or not. This will effect the rendering data which needs to wait until request is resolved. Otherwise we might fix that by having a loading skelaton for the circle or just hide it until request is resolved (its open for discussion).
- the style can be improved as well (open for any suggestions).
- I tried to make DataTable and Modal as generic as possible thus, it can be used if needed
src/components/HungerAlert/style.css
Outdated
| @@ -0,0 +1,44 @@ | |||
| .pulse { | |||
There was a problem hiding this comment.
as already mentioned we do not use css and use tailwind instead
There was a problem hiding this comment.
yeah I wish I can animate and have pseudo-elements in tailwind as well but I didnt find any.
|
Hi, looks like you added your own table component. I was actually working with tables (but there's no PR yet because I am waiting for other stuff) - I started out making a more complex table that could be generalized into a simple table as yours. It might be good to merge both implementations later, but maybe it's okay for now to have one simpler and one more complex table in parallel, since both are using the NextUI table and have roughly the same style. My table (branch f-38-implement-glossary-and-methodology-pages): |
|
One remark regarding the styling: To me it's a bit irritating that the list modal changes its size if I reach page 3. I'd expect it to have a constant height (i.e. the height needed for 10 rows) and have more white space on the last page, maybe except for the case when there's only one page in total. |
| const countryData = await container | ||
| .resolve<GlobalDataRepository>('GlobalDataRepository') | ||
| .getMapDataForCountries(); |
There was a problem hiding this comment.
you shouldn't download this data here, because this is already downloaded on the server in page.tsx. You can pass that data to this component. However, you would have to move this component to the page.tsx file, but I don't think that's a problem, since we will only have one page. Or you could fetch the data in the layout.tsx as well, I think next.js is smart enough not to make the same call twice.
There was a problem hiding this comment.
The problem with this endpoint that it returns this error when its called from server side componets =>
{
"status": "resolved_model",
"value": "{"error":"Error retrieving data: Error fetching ADM0 data: ('Connection broken: IncompleteRead(1604 bytes read, 8636 more expected)', IncompleteRead(1604 bytes read, 8636 more expected))"}",
"reason": null,
"_response": {
"_bundlerConfig": null,
"_moduleLoading": null,
"_chunks": {},
"_stringDecoder": {},
"_rowState": 0,
"_rowID": 0,
"_rowTag": 0,
"_rowLength": 0,
"_buffer": []
},
"_debugInfo": null
}
but when its called from client side component this error never occures. I have tried different endpoints on server side component and this issue never happens , I am not sure how exactly next.js handles those requests on different rendering components. so the work around that I have done is to pass data from layout.js (which always returns the above error) if it returns error just call the endpoint again from the client side which eventually will be resolved without an error.
There was a problem hiding this comment.
oh I think this happens because a few days ago, their API was broken and this response was returned, and next cached it locally. When you make the request now, it returns the correct data, but it's over 2 MB so it can't update the cache with that. You can fix this by deleting the .next folder in the root dir, and then running yarn dev again.
✅ Deploy Preview for wfp-hungermap ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…tech data from parent
Yes that's true. Its annoying when navigating between pages with different hights, but I have fixed it now. @jschoedl |
There was a problem hiding this comment.
LGTM! Had few aspects that should have been fix, but that's fine. Next time please make sure to add a Skeleton Loading Indicator when you are dependent on data fetching. Nevertheless keep in mind, that we should not have two custom data tables, so align with @jschoedl there later
