-
Notifications
You must be signed in to change notification settings - Fork 31
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
Shippensburg Land Projections: Add to Land Cover Modification Dialog #3344
Conversation
Just like the Future Weather options, these are only available for selection within the DRB. Outside, they are disbabled.
Adds a dropdown at the top of the land cover dialog which lists available presets. Selecting one of the non-default presets fetches the value from the related analysis, converts it into a Mapshed format, and populates the text boxes. Any existing text box values will be overwritten. The total is updated accordingly. In some cases, the total doesn't quite completely match the value of NLCD. In these cases, the user must adjust the numbers manually. On updating the numbers, they are saved as if the user had input them manually, replicating the existing behavior.
Previously we extracted relevant bits from the scenario and passed in only those to the Land Cover Dialog. Now, as it may need to remove modifications, instead of passing in further bits, we now pass in the entire scenario. This allows for more flexibility within the Land Cover Dialgo. Also refactor out App import, which tends to tie things up.
f184737
to
333d11b
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. The non-matching sum when switching between land cover sources is a bummer, but any "fix" would have to be arbitrary, so I think leaving it is appropriate. I could expect some feedback from the client about giving it a particular behavior though. I tested through a number of scenarios, including switching unit preferences mid-way, and it always showed up correctly.
I was able to get in a sequence of the app not thinking I was in the DRB on some large hand drawn shapes in SE PA (and disabling the dropdown), but I couldn't reliably reproduce it after a while. I may have clipped the DRB boundary without realizing it. If it crops up for you at all, it may be worth looking into further. Future issues look good too. 👍
Thanks for taking a look! I added #3345 to add the DRB test polygon as a visualizable layer, to help debug this both for us and the users, since there is now much DRB-specific stuff in the app. |
Overview
Adds a dropdown to the Land Cover Modification Dialog to auto-populate the fields with values from the selected Land Cover analysis results. This takes advantage of the current implementation, with minor edits to populate the values and remember which preset was used.
The connecting issue mentions two implementation details that are deferred for later:
Connects #3333
Demo
Notes
Instead of adding another endpoint to fetch the values from the Drexel API again, I chose to reuse the values already fetched during analysis. This creates a new dependency on Analyze results, saves a lot of code.
The area total for the DRB numbers does not match exactly (because the NLCD numbers come from our own geoprocessing service and the DRB numbers from from Drexel's fast zonal API). I could not come up with a way to resolve this discrepancy, and is left to the user to manage.
Once you save Land Cover modifications, wait for the results to save, and refresh the page, it calculates the results again. This is existing behavior, logged in #3343.
The code for getting the appropriately modified GMS for a scenario is currently spread across many functions, and needs to be unified. I'll make a follow up
enhancement
card for that.Testing Instructions
bundle