-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix: Polygon simplification on E&A page #1268
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
hm, I'm not able to get anything to load. Here are the files I tried: Alabama_State_Boundary-shp.zip |
@aboydnw Thanks for testing and catching the bug that I pushed at the last minute 😓 Please test it again when you have a chance. |
This seems to be working much better now! The shape for Indianapolis still doesn't show exactly accurate, but now it seems to be because rings are not supported, which I think is an acceptable limitation. I was able to upload a fairly complex shapefile and it worked well. I think this should give folks at AGU more ability to find a shapefile that works @Jeanne-le-Roux @slesaad |
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.
From a manual testing perspective, looks good to me.
2b7a016
to
ce5afa7
Compare
ce5afa7
to
c3138aa
Compare
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.
Looks great @hanbyul-here, thanks for adding the tests as well! Tested using a few .shp and geojson files and can't spot any issues with the fixes.
app/scripts/components/common/map/controls/hooks/use-custom-aoi.tsx
Outdated
Show resolved
Hide resolved
app/scripts/components/common/map/controls/hooks/use-custom-aoi.tsx
Outdated
Show resolved
Hide resolved
### Description of Changes Increase the limit of the number of each aoi polygon. Separate out functions that handles the aoi validation for easy test and better readability. Fix error handling. Add unit test for aoi validation.
## What am I changing and why Updating UI to v5.11.1 ## How to test v5.11.1 adds the fix for AOI simplification on E&A page to v5.11.0 - details for AOI simplification fix is NASA-IMPACT/veda-ui#1268 ##⚠️ Checks - [x] I have confirmed that [updating the `veda-ui` submodule](https://github.com/NASA-IMPACT/veda-config-ghg/blob/main/docs/DEVELOPMENT.md#development) is needed and **only done so** if that's the case.
Close #1267, Close US-GHG-Center/veda-config-ghg#648
Description of Changes
Increased the limit of the number of each polygon.
Separated out a function that handles the aoi validation for an easy test and better readability.
Fix error handling.
Notes & Questions About Changes
The number that I ended up with is an outcome of my experiment with our GHG endpoint, details are in : US-GHG-Center/veda-config-ghg#648 (comment)
I think our current setup was more for multiple simple polygons - and this setup is more for 1 complicated polygon (which also hopefully covers the multiple simple polygons case.)
Validation / Testing
We should test with GHG endpoint because of AWS setup there, I made a preview link : https://deploy-preview-664--ghg-demo.netlify.app/exploration